1. Einstein Toolkit Avatar Einstein Toolkit
  2. Einstein Toolkit
  3. tickets
  4. Issues

Multipole: Add possibility to have out_dir distinct from regular IO:out_dir, but default to the old behavior

Issue #1955 closed
Frank Löffler created an issue

https://bitbucket.org/einsteintoolkit/einsteinanalysis/pull-requests/2/add-possibility-to-have-out_dir-distinct/diff

Keyword:

Comments (10)

  3. Roland Haas
    • changed status to open
    • removed 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.

  4. Frank Löffler reporter
    • changed status to open
    • removed 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 ...

  5. Roland Haas
    • changed status to open
    • removed 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).

  6. Frank Löffler reporter
    • removed comment

    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.

  7. Roland Haas
    • removed comment

    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.

  8. Frank Löffler reporter
    • removed comment

    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...

  11. Log in to comment
Assignee
–
Type
enhancement
Priority
minor
Status
closed
Component
EinsteinToolkit thorn
Milestone
–
Version
development version
Votes
0
Watchers
0
Jira Software: the preferred issue tracker for Bitbucket. Join the team!