Hello all,
Log:
- remove option to have different gamma and K for different stars. That's physically impossible.
- adjust test par files (will do so with GRHydro next)
- modify the way solutions are added so that atmosphere cuts are applied after solutions are added. Otherwise one of the stars will have the atmosphere added to it. This breaks symmetry.
This commit (and the following ones) make the tests fail, at least in subversion (Zelmani is ok since the author *does* provide updated tests, just not in GRHydr/svn, so I could port over the change).
I am personally generally unhappy about commits that require everybody to change/update their paramater files unless the change is required to prevent a bug from manifesting. Removing "[0]" from ones parameter file is not hard, but will break each and every single one of them. One of the nice things about Cactus has been that (some parts of it) are fairly stable and developers take care to not introduce changes that require each user to update their parameter files (unless the user wants/needs new features).
I am also not sure I agree with the statement that one cannot have different Gama or K for different stars. Even if the same Gamma and K would be needed for all neutron stars (which I also don't fully understand, why should not a different Gamam or K be a better approximiation to NS of different massses?) there is still the possiblilty of setting up two different sorts of stars and evolve those. Note that I make no statement how to proceed with such an evolution past the point where the stars would touch. To my knowledge only Ian Hawke did such simulations in the past, and Ian effective advected the EOS parameters with the fluid if I remember correctly. If someone with more expterise would comment that would be helpful.
I would like to revert parts of this patch (keeping the atmosphere treatment that prevents asymetries in the data setup and seems to be currently buggy) and restore the possibility to have different K and Gamma (indep of whether this is physical I don't see a point in removing this functionality).
Please comment (in private or on the mailing list).
Yours, Roland
Hi,
On Sun, Jul 07, 2013 at 11:06:26AM -0700, Roland Haas wrote:
This commit (and the following ones) make the tests fail, at least in subversion (Zelmani is ok since the author *does* provide updated tests, just not in GRHydr/svn, so I could port over the change).
Regardless of what happens here: the tests should be updated accordingly if this is necessary.
I am personally generally unhappy about commits that require everybody to change/update their paramater files unless the change is required to prevent a bug from manifesting. Removing "[0]" from ones parameter file is not hard, but will break each and every single one of them. One of the nice things about Cactus has been that (some parts of it) are fairly stable and developers take care to not introduce changes that require each user to update their parameter files (unless the user wants/needs new features).
In general, I agree, but sometimes I see that changes are necessary, even without bugs being involved. A code cleanup from time to time is good and might involve some changes - in moderation of course.
I am also not sure I agree with the statement that one cannot have different Gama or K for different stars.
I guess the argument is that there is only _one_ EOS in physics in theory. Of course, what we have here is an approximation, and different stars might use different approximations, resulting in the same functional form of a polytropic EOS, but with different parameters. Something like a piecewise polytropic EOS might be another solution for this problem, but why should I dictate someone else's choice. Also, someone might want to perturb one of the stars but not another by changing its EOS initially.
I would like to revert parts of this patch (keeping the atmosphere treatment that prevents asymetries in the data setup and seems to be currently buggy) and restore the possibility to have different K and Gamma (indep of whether this is physical I don't see a point in removing this functionality).
I agree. Could you provide a patch? Having said that, I would like to use the opportunity to thank Christian for working on this. The TOVSolver is indeed in dire need for cleanup and I very much welcome him working on it. I hope the correct "translation" of "Wo gehobelt wird, fallen Späne" is really "You can't make an omelet without breaking eggs", but it is certainly the same spirit.
Frank
On 7 Jul 2013, at 20:27, Frank Loeffler knarf@cct.lsu.edu wrote:
Hi,
On Sun, Jul 07, 2013 at 11:06:26AM -0700, Roland Haas wrote:
This commit (and the following ones) make the tests fail, at least in subversion (Zelmani is ok since the author *does* provide updated tests, just not in GRHydr/svn, so I could port over the change).
Regardless of what happens here: the tests should be updated accordingly if this is necessary.
Did you know that the tests were going to fail before committing? i.e. was it a conscious decision to break the tests in order to get the code committed sooner?
In some cases (e.g. the failures in TwoPunctures), I think there are NaNs generated. Is this an indication that the commits themselves were wrong?
There is a nonzero cost to having the tests failing. Any new failures are less likely to be noticed, and it makes work for people who then have to investigate why the tests fail. Ideally, developers would not commit code which breaks the tests. It's good to have the automated tests running as a safety net, but I think developers can still be expected to run the tests before making a huge series of commits like this.
I am personally generally unhappy about commits that require everybody to change/update their paramater files unless the change is required to prevent a bug from manifesting. Removing "[0]" from ones parameter file is not hard, but will break each and every single one of them. One of the nice things about Cactus has been that (some parts of it) are fairly stable and developers take care to not introduce changes that require each user to update their parameter files (unless the user wants/needs new features).
In general, I agree, but sometimes I see that changes are necessary, even without bugs being involved. A code cleanup from time to time is good and might involve some changes - in moderation of course.
I am undecided. I agree that it is pleasant to tidy things up and make the current version clean. But at the same time you have to consider the disadvantages. It is very frustrating when you are trying to locate where a bug was introduced with historical versions of the code if you can't use the same parameter file with old and new versions. If at all possible, I would try to make the old parameter files still work. If we had 100% test coverage (i.e. all the code was exercised during the tests), then maybe we would have the luxury of assuming that we never had to go back to old versions to debug where someone broke something, but we are nowhere near that, so it is inevitable that we will have to do this from time to time, and if the code is not backwards compatible, this becomes somewhat of a crapshoot. Often you can get the best of both by introducing a new parameter rather than changing the old one, though I haven't looked into the details of this particular issue so I don't know if that makes sense here.
Hello all,
I am undecided. I agree that it is pleasant to tidy things up and make the current version clean. But at the same time you have to consider the disadvantages. It is very frustrating when you are trying to locate where a bug was introduced with historical versions of the code if you can't use the same parameter file with old and new versions. If at all possible, I would try to make the old parameter files still work. If we had 100% test coverage (i.e. all the code was exercised during the tests), then maybe we would have the luxury of assuming that we never had to go back to old versions to debug where someone broke something, but we are nowhere near that, so it is inevitable that we will have to do this from time to time, and if the code is not backwards compatible, this becomes somewhat of a crapshoot. Often you can get the best of both by introducing a new parameter rather than changing the old one, though I haven't looked into the details of this particular issue so I don't know if that makes sense here.
Based on that it having different K (or Gamma) for different NS should not be done since it is not physical (and no one objects to removing this facility) then one way to keep most old parfiles (for single NS, for binary NS we are changing results since the code changes) would be to keep TOV_K a vector but reduce its size to 1. This will make the parfile parser throw an error when the other parameters are set but leave single star files alone (which are the majority of files out there I hope since binaries are not constraint satisfying). This is somewhat ugly since it means that there is a single parameter but it appears as a vector. It would thus most likely be best to eventually change the parameter to the way they are right now (after deprecating the current one). A further alternative is to leave the ability for multiple K/Gamma values, unify the atmosphere treatment in TOVSolver (ie let GRHydro do it) and put a long comment as to the dangers of setting multiple K into param.ccl. "Consider yourself warned ...".
I'll push patches for the remaining triviail failures (Trigger and Outflow). There is one slightly non-trivial failure in TOVSolver itself which requires a bit of digging into the code.
Yours, Roland
On 8 Jul 2013, at 12:11, Roland Haas rhaas@tapir.caltech.edu wrote:
Hello all,
I am undecided. I agree that it is pleasant to tidy things up and make the current version clean. But at the same time you have to consider the disadvantages. It is very frustrating when you are trying to locate where a bug was introduced with historical versions of the code if you can't use the same parameter file with old and new versions. If at all possible, I would try to make the old parameter files still work. If we had 100% test coverage (i.e. all the code was exercised during the tests), then maybe we would have the luxury of assuming that we never had to go back to old versions to debug where someone broke something, but we are nowhere near that, so it is inevitable that we will have to do this from time to time, and if the code is not backwards compatible, this becomes somewhat of a crapshoot. Often you can get the best of both by introducing a new parameter rather than changing the old one, though I haven't looked into the details of this particular issue so I don't know if that makes sense here.
Based on that it having different K (or Gamma) for different NS should not be done since it is not physical (and no one objects to removing this facility) then one way to keep most old parfiles (for single NS, for binary NS we are changing results since the code changes) would be to keep TOV_K a vector but reduce its size to 1. This will make the parfile parser throw an error when the other parameters are set but leave single star files alone (which are the majority of files out there I hope since binaries are not constraint satisfying). This is somewhat ugly since it means that there is a single parameter but it appears as a vector. It would thus most likely be best to eventually change the parameter to the way they are right now (after deprecating the current one). A further alternative is to leave the ability for multiple K/Gamma values, unify the atmosphere treatment in TOVSolver (ie let GRHydro do it) and put a long comment as to the dangers of setting multiple K into param.ccl. "Consider yourself warned ...".
I'll push patches for the remaining triviail failures (Trigger and Outflow). There is one slightly non-trivial failure in TOVSolver itself which requires a bit of digging into the code.
Thanks. We are now down to 4 failing tests:
https://build.barrywardell.net/job/EinsteinToolkit/657/testReport/
Do you know why these are failing?
On Thu, Jul 11, 2013 at 11:09:53AM +0200, Ian Hinder wrote:
Thanks. We are now down to 4 failing tests:
Do you know why these are failing?
I committed changes that should fix both. Two were adapted in a slightly wrong way (setting parameters twice, which is now an error even if it is set to the same value), and two weren't adapted yet.
Frank
Hello all,
I committed changes that should fix both. Two were adapted in a slightly wrong way (setting parameters twice, which is now an error even if it is set to the same value), and two weren't adapted yet.
Not quite. The test_two_av test actually sees changes in test values so will continue to fail (just with failures in data comparason rather than parfile errors). The TwoPunctures test will also continue to fail since one of the 25 ptaches from the last GRHydro update breaks it (only runs on 1 process which apparently I did not test).
Yours, Roland
On Thu, Jul 11, 2013 at 03:49:06PM +0200, Roland Haas wrote:
Not quite. The test_two_av test actually sees changes in test values so will continue to fail (just with failures in data comparason rather than parfile errors).
Do we know why?
The TwoPunctures test will also continue to fail since one of the 25 ptaches from the last GRHydro update breaks it (only runs on 1 process which apparently I did not test).
Weren't these patches committed individually, which should make Jenkins useful for finding out which of these broke things?
Frank
Hello all,
Weren't these patches committed individually, which should make Jenkins useful for finding out which of these broke things?
I believe Jenkins only tracks on the granularity level of commits to the master git repository. Certainly the "failed since" link (https://build.barrywardell.net/job/EinsteinToolkitProposed/453/testReport/(r...) in the emails points to a comitt that incorporates multiple svn commits.
It should should not be difficult to walk the list of commits and check where it fails though, I just don't have the time to do so right now. Certainly anyone who wants to give it a try is welcome. :-)
Yours, Roland
On Thu, Jul 11, 2013 at 05:21:08PM +0200, Roland Haas wrote:
I believe Jenkins only tracks on the granularity level of commits to the master git repository.
Me too, but I (probably incorrectly) assumed that the git repository would create one commit for every commit happening in the ET, and thus would test every change separately.
It should should not be difficult to walk the list of commits and check where it fails though, I just don't have the time to do so right now.
Not difficult - sure, but it takes time. Ideally it should take the time of the one committing the changes. Anyway - it's r555 of GRHydro: add grid function for sqrt(detg).
Based on the commit message my best guess would be that GRHydro isn't recalculating the new GF when the metric changed by TwoPunctures, or it might even not be initialized yet at that point. To figure that out I leave to the authors of the commit who undoubtly know better what they did than me, and could fix this faster than me.
Frank
On 2013-07-11, at 12:54 , Frank Loeffler knarf@cct.lsu.edu wrote:
On Thu, Jul 11, 2013 at 05:21:08PM +0200, Roland Haas wrote:
I believe Jenkins only tracks on the granularity level of commits to the master git repository.
Me too, but I (probably incorrectly) assumed that the git repository would create one commit for every commit happening in the ET, and thus would test every change separately.
It should should not be difficult to walk the list of commits and check where it fails though, I just don't have the time to do so right now.
Not difficult - sure, but it takes time. Ideally it should take the time of the one committing the changes. Anyway - it's r555 of GRHydro: add grid function for sqrt(detg).
In case this grid function is expected to be a performance improvement:
I would guess that calculating sqrt(det(g_ij)) takes about 20 to 30 cycles, if the 3-metric g_ij is in the D1 cache, i.e. if the 3-metric is already used in the same loop. Accessing a grid function element that is stored in memory (assuming it remains in the L3 cache) costs about 50 cycles.
Of course, the details will vary between systems, and will depend on which cache level holds the data, and what optimizations the compiler can apply to the loop. Don't take these numbers at face value. The point here is that, although sqrt may "look expensive", it may well be cheaper to re-calculate than to pre-calculate and store it.
-erik
Hello Erik, Frank,
In case this grid function is expected to be a performance improvement:
I was intended as a performance improvement. The changes make the code faster though I do not know if only the sum total of all changes that happened to prim2con etc make it faster or if already the sqrtdetg part makes it faster.
I would guess that calculating sqrt(det(g_ij)) takes about 20 to 30 cycles, if the 3-metric g_ij is in the D1 cache, i.e. if the 3-metric is already used in the same loop. Accessing a grid function element that is stored in memory (assuming it remains in the L3 cache) costs about 50 cycles.
Interesting I had not realized that a sqrt is actually faster than a memeory access (multiplications and additions: yes of course, divisions: I wouldn't have known). So I guess what one has to do is actually run a test that just changes this one aspect and whatever the result is, documennt that in the code.
Looking at the code it might be possible that the postiitve effec comes from avoiding multiple calls to sqrt for the same argument and/or from passing sqrt(detg) instead of detg to the prim2con and con2prim routines. Some testing seems in order.
Of course, the details will vary between systems, and will depend on which cache level holds the data, and what optimizations the compiler can apply to the loop. Don't take these numbers at face value. The point here is that, although sqrt may "look expensive", it may well be cheaper to re-calculate than to pre-calculate and store it.
ok. I'll test them (or see if I can talk someone else into testing them).
Yours, Roland
On 11 Jul 2013, at 17:21, Roland Haas rhaas@tapir.caltech.edu wrote:
Hello all,
Weren't these patches committed individually, which should make Jenkins useful for finding out which of these broke things?
I believe Jenkins only tracks on the granularity level of commits to the master git repository. Certainly the "failed since" link (https://build.barrywardell.net/job/EinsteinToolkitProposed/453/testReport/(r...) in the emails points to a comitt that incorporates multiple svn commits.
There are two issues here. The first is that several quick commits to the SVN repository will be collapsed into a single git super-repository commit, and the second is that Jenkins is only able to test the "current" state, it doesn't walk through each commit of the repository testing each one.
I added an option to create one commit per submodule commit in the super-repo to the git-module tool a while ago, but I'm not sure it is wise to use this option. The problem is that it gives the impression that a particular super-repo commit represents a snapshot in time that actually existed, whereas that particular snapshot may never have existed. For example, someone might have pushed a set of 10 commits in one go to a submodule repository (e.g. Carpet), with later commits fixing bugs identified in earlier commits. The earlier commits were never the HEAD of the central repository, and might never have existed at the same time as the current version of other repositories. It just wouldn't make sense to have a super-repo commit representing that state. There is also the issue of having branching in the repositories. Suppose the master branch in the sub repository diverges and converges (e.g. because you did a pull without rebase). Which side of the branch do you test? Both? In the end, I decided that it made more sense to create one commit for each "snapshot" in time that was actually found as the current HEADs of all the sub repositories, and that is the current behaviour.
The ability to test every commit is just missing in Jenkins. Probably it has not been widely requested by users because it is usually quite clear which of a small number of commits is responsible for a given breakage, and testing every commit might use too many resources. It would be possible to hack something on top of Jenkins to achieve this. On the other hand, developers should also be capable of running the tests themselves, and it's only a small number of commits which need testing here.
It should should not be difficult to walk the list of commits and check where it fails though, I just don't have the time to do so right now. Certainly anyone who wants to give it a try is welcome. :-)
Weren't you the one who made the commits in the first place? I think if you don't have time to fix them, you might not want to commit them… :p Revert?
Hello all,
Weren't you the one who made the commits in the first place? I think if you don't have time to fix them, you might not want to commit them… :p Revert?
Yes (to my shame). I did run the 2 processor tests before proposing the patches and just before pushing them (having been burned like this before). I did not however run the 1proc tests. I would normally be a bit fast er fixing this, yes, though nornmally I am also not at a conference having to prepare a talk for the next day :-). I am not sure if reverting all 25 patches (since the depend on each other) is not actually more work than just fixing the scheduling (which is alomost certainly all there is to it).
Yours, Roland
Hello all,
On Thu, Jul 11, 2013 at 03:49:06PM +0200, Roland Haas wrote:
Not quite. The test_two_av test actually sees changes in test values so will continue to fail (just with failures in data comparason rather than parfile errors).
Do we know why?
Not really. I suspect (from what code changed but without having tested this in any way), that there is an issue between applying atmosphere to the average density and each density then averaging. Just a guess though.
Yours, Roland
Hi Roland,
the EOS is universal. If one of the stars has a smaller mass, then it has a smaller central density. If one wants to do exotic physics, then one needs a different evolution code than GRHydro anyways. I see no reason why the TOV solver should support something that can't be evolved by the ET in any case and just adds complication and can trick users that do not have sufficient experience into doing something massively stupid.
Also, we are in a development phase, so it's expected that things break once in a while.
- Christian
On 7/7/13 11:06 AM, Roland Haas wrote:
Hello all,
Log:
- remove option to have different gamma and K for different stars. That's physically impossible.
- adjust test par files (will do so with GRHydro next)
- modify the way solutions are added so that atmosphere cuts are applied after solutions are added. Otherwise one of the stars will have the atmosphere added to it. This breaks symmetry.
This commit (and the following ones) make the tests fail, at least in subversion (Zelmani is ok since the author *does* provide updated tests, just not in GRHydr/svn, so I could port over the change).
I am personally generally unhappy about commits that require everybody to change/update their paramater files unless the change is required to prevent a bug from manifesting. Removing "[0]" from ones parameter file is not hard, but will break each and every single one of them. One of the nice things about Cactus has been that (some parts of it) are fairly stable and developers take care to not introduce changes that require each user to update their parameter files (unless the user wants/needs new features).
I am also not sure I agree with the statement that one cannot have different Gama or K for different stars. Even if the same Gamma and K would be needed for all neutron stars (which I also don't fully understand, why should not a different Gamam or K be a better approximiation to NS of different massses?) there is still the possiblilty of setting up two different sorts of stars and evolve those. Note that I make no statement how to proceed with such an evolution past the point where the stars would touch. To my knowledge only Ian Hawke did such simulations in the past, and Ian effective advected the EOS parameters with the fluid if I remember correctly. If someone with more expterise would comment that would be helpful.
I would like to revert parts of this patch (keeping the atmosphere treatment that prevents asymetries in the data setup and seems to be currently buggy) and restore the possibility to have different K and Gamma (indep of whether this is physical I don't see a point in removing this functionality).
Please comment (in private or on the mailing list).
Yours, Roland
Users mailing list Users@einsteintoolkit.org http://lists.einsteintoolkit.org/mailman/listinfo/users
users@lists.einsteintoolkit.org
-
Christian D. Ott -
Erik Schnetter -
Frank Loeffler -
Ian Hinder -
Roland Haas -
Roland Haas