[Einstein Toolkit] #1955: Multipole: Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior
#1955: Multipole: Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior -----------------------------------+---------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: EinsteinToolkit thorn | Version: development version Keywords: | -----------------------------------+---------------------------------------- https://bitbucket.org/einsteintoolkit/einsteinanalysis/pull-requests/2 /add-possibility-to-have-out_dir-distinct/diff
#1955: Multipole: Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior ------------------------------------+--------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: ------------------------------------+---------------------------------------
Comment (by rhaas):
Would you like to have this reviewed? The status of the ticket is "new".
#1955: Multipole: Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior ------------------------------------+--------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: review Priority: minor | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: ------------------------------------+--------------------------------------- Changes (by knarf):
* status: new => review
#1955: Multipole: Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior ------------------------------------+--------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: reopened Priority: minor | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: ------------------------------------+--------------------------------------- Changes (by rhaas):
* status: review => reopened
Comment:
Seems like a good idea to me. However:
* the commit should start with Multipole: * Multipole needs to create its output directory. This needs to be done on the rank that creates the file afterwards.
without these the pull request should not be applied.
#1955: Multipole: Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior ------------------------------------+--------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: review Priority: minor | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: ------------------------------------+--------------------------------------- Changes (by knarf):
* status: reopened => review
Comment:
A new version was pushed that creates the directories. Only process 0 does output here, so only this process creates the directory. Concerning commit message: yes, but git doesn't allow (discourages) changes to the commit message once pushed, as it rewrites history. I suggest to give the merge commit the respective description line, which should be good enough, given this is on a separate, short-lived branch. I'll try to remember next time ...
#1955: Multipole: Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior ------------------------------------+--------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: reviewed_ok Priority: minor | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: ------------------------------------+--------------------------------------- Changes (by rhaas):
* status: review => reviewed_ok
Comment:
I added some more comments. Basically:
* you could use CCTK_VERROR instead of CCTK_VWarn (so that the compiler knows that the function does not return) * please report the error code to the user
since the pull request is not on a branch in a public repo, you could do "git rebase -i" to change the commit messages without losing anything. With a merge your commit as is will show up on master, not just the single merge commit (as it would in svn).
#1955: Multipole: Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior ------------------------------------+--------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: reviewed_ok Priority: minor | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: ------------------------------------+---------------------------------------
Comment (by knarf):
I avoided CCTK_VERROR because it is new. It would make the thorn depend on a recent flesh version just for that macro.
I don't understand what you mean with "not on a branch in a public repo". The pull request is in a branch in https://bitbucket.org/einsteintoolkit/einsteinanalysis, which is the main, public ET repository. I do understand that the single commits still show up as-is after the merge, but the merge commit message containing "Multipole: xxx" should make it sufficiently clear what happened in that merge.
#1955: Multipole: Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior ------------------------------------+--------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: reviewed_ok Priority: minor | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: ------------------------------------+---------------------------------------
Comment (by rhaas):
Even when adding "Multipole:" the merge request, git log still is lacking the required information: {{{ $ git log commit 0ef3e71f999c3f7cf2f925989595b86bf7d53e20 Merge: cd658d7 c25f761 Author: Roland Haas rhaas@aei.mpg.de Date: Tue Aug 30 14:46:04 2016 -0500
Multipole: Merge branch own_io_dir
commit c25f7615292e7bacdb652001e1ad826ba5984277 Author: Frank Löffler knarf@cct.lsu.edu Date: Tue Aug 30 11:47:34 2016 -0500
Multipole: create out_dir if it does not exist
commit 953e5aac90e58f61d8d66c72fa727ea73410f088 Author: Frank Löffler knarf@cct.lsu.edu Date: Mon Aug 29 16:56:57 2016 -0500
initialize my_out_dir with correct value
commit d61c3ad38f49a74a3c3832b0417c0acb9619d9a0 Author: Frank Löffler knarf@cct.lsu.edu Date: Mon Aug 29 13:01:34 2016 -0500
Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior
commit cd658d7a1cfee286c611cb4e171e99aefbdb07ef Author: Erik Schnetter schnetter@gmail.com Date: Thu Jul 21 16:52:47 2016 -0400
EHFinder: Correct error in division
}}} is what it looks like (see https://bitbucket.org/rhaas80/einsteinanalysis).
There is {{{CCTK_VError}}} which is quite old (at least in the current released version) so should be safe to use (compared to {{{CCTK_VERROR}}}) and still lets the compiler know that the routine won't return.
The branch you created was a "private branch" I thought, not really intended for use by end-users. Is that correct? In that case you could as well (if eg you had not write permission to einsteinanalysis) have forked the repo (into one of your own), done the modification there and then created a pull request? So since the branch (seems to) have served no other purpose than to allow for the pull request, I would treat its history as something that does not need to be preserved (contrary to eg master's or the release branches' history which must never change).
Note: these are both optional (since they only affect developers), returning the error code to the user would be more important to me to tell the truth.
#1955: Multipole: Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior ------------------------------------+--------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: reviewed_ok Priority: minor | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: ------------------------------------+---------------------------------------
Comment (by knarf):
True about 'git log'. The history would be a little better visible in a GUI layout, which is what I prefer to look at non-linear git histories.
I'll look into CCTK_VError.
Concerning branch: I created the branch to make the code 'public', visible for a review, and easily accessible using git. Of course, such a branch isn't meant for 'end users', but developers. I could have forked the repo for the same purpose. Branch vs. fork for a feature request is mostly a taste choice. Since I can create branches on the original repository I prefer that over a fork which a reviewer would have to add manually for a review, and which I would have to remove after it was merged. Someone without write access would fork instead. Of course, there is also those that advocate to use both a fork and then a new branch within that...
#1955: Multipole: Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior ------------------------------------+--------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: closed Priority: minor | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: fixed | Keywords: ------------------------------------+--------------------------------------- Changes (by knarf):
* status: reviewed_ok => closed * resolution: => fixed
Comment:
Merged. Also output error code in error case.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit