AHFinderDirect uses incorrect origin_* upon recovery when horizon was never found before

Comments (9)

1. 

   Roland Haas

   • removed comment

   Three comments: * there is commented out code in the commit which might as well be not committed :-) * this code seems to pose the danger of ps.origin() never being called when both h_found_flag[horizon_number-1] == 0 and (ah_really_initial_find_flag[horizon_number-1] . Since before ps.origin() was always called, I wonder if this might cause it to remain initialized. * finally the code in the else branch uses horizon_number instead of horizon_number-1

   A general question: This seems to be an instance of a thorn that makes copies of Cactus' parameters (or quantities computed from them) and then checkpoints them. This seems to interfere badly with Cactus' ability to change parameter values on recovery. My question is if there is use in such thorns trying to emulate the logic in Cactus as to when to update values? I believe the login in Cactus goes something like this: at recovery the values for parameters are those stored in checkpoints unless it is explicitly set in the new parameter file and this setting differs from the one in the original parameter file used at checkpoint time in which case values from the new parameter file are used. Is this correct? Do we want to implement something similar in AHFinderDirect (I assume this to involve checking how often a parameter has been set at POST_RECOVER_VARIABLES time).

   • 2013-02-01T01:12:43+00:00
2. 

   Roland Haas

   • changed status to open
   • removed comment

   • 2013-02-01T01:17:15+00:00
3. 

   anonymous

   • removed comment

   Replying to [comment:1 rhaas]:

   Three comments: * there is commented out code in the commit which might as well be not committed :-) * this code seems to pose the danger of ps.origin() never being called when both h_found_flag[horizon_number-1] == 0 and (ah_really_initial_find_flag[horizon_number-1] . Since before ps.origin() was always called, I wonder if this might cause it to remain initialized.

   I think the new behavior is correct. If a horizon has never been found, then its ps.origin is just at whatever was there in the initial par-file (it got set to this value initially). If one wants to change the origin parameter upon recovery (the parameter is steerable= recovery!), this new steered parameter value would never be used! This is desasterous when one decides to try to find a horizon elsewhere. Remark: I picked the condition such that it matches the condition for when the initial_guess_ellipsoid routine is called (which is causing all the trouble).

   Once a horizon was found, one could argue that instead of using the parameter value of origin_*, it should take the origin value that was stored in the checkpoint. Actually, I think this behavior is more correct, because without pretracking or tracking from a grid scalar, we rely on the fact that the old origin is not too far from the anticipated new origin (at least I believe that this is the case). If we would take the value from the parfile upon recovery, the user would have to set the value according to where the horizon was found the last time! This is clearly to much of a hassle.

   • finally the code in the else branch uses horizon_number instead of horizon_number-1

   Yes, but not for parameters. Jonathan starts counting from 1.

   A general question: This seems to be an instance of a thorn that makes copies of Cactus' parameters (or quantities computed from them) and then checkpoints them. This seems to interfere badly with Cactus' ability to change parameter values on recovery. My question is if there is use in such thorns trying to emulate the logic in Cactus as to when to update values? I believe the login in Cactus goes something like this: at recovery the values for parameters are those stored in checkpoints unless it is explicitly set in the new parameter file and this setting differs from the one in the original parameter file used at checkpoint time in which case values from the new parameter file are used. Is this correct? Do we want to implement something similar in AHFinderDirect (I assume this to involve checking how often a parameter has been set at POST_RECOVER_VARIABLES time).

   • 2013-02-01T01:28:18+00:00
4. 

   Roland Haas

   • removed comment

   ok to the third reply (on numbering). Ok to the second reply in that the proposed behaviour is more correct. My worry was that the patchsystem might be left unitialized, reading the code however reveals that the patchsystem is unconditionally initialized with the origin_[yxz] parameters in AHFinderDirect_setup. It thus seems as if indeed the assignment in the "else" branch is superfluous but harmless since it will never change the values from those already set in AHFinderDirect_setup. Is this correct?

   Otherwise ok to apply (possibly sans comment and possibly sans the "else" branch).

   • 2013-02-01T01:48:29+00:00
5. 

   Christian Reisswig reporter

   • removed comment

   Replying to [comment:4 rhaas]:

   ok to the third reply (on numbering). Ok to the second reply in that the proposed behaviour is more correct. My worry was that the patchsystem might be left unitialized, reading the code however reveals that the patchsystem is unconditionally initialized with the origin_[yxz] parameters in AHFinderDirect_setup. It thus seems as if indeed the assignment in the "else" branch is superfluous but harmless since it will never change the values from those already set in AHFinderDirect_setup. Is this correct?

   Otherwise ok to apply (possibly sans comment and possibly sans the "else" branch).

   Agreed. I have removed and tested it without the else branch and it works fine. I can commit my changes.

   • 2013-02-01T15:03:32+00:00
6. 

   Roland Haas

   • changed status to open
   • removed comment

   • 2013-02-01T15:29:54+00:00
7. 

   Roland Haas

   • removed comment

   Please apply.

   • 2013-02-01T15:30:09+00:00
8. 

   Roland Haas

   • changed status to resolved
   • removed comment

   Applied as rev 1572 of AHFinderDirect.

   • 2013-02-01T23:48:46+00:00
9. 

   Roland Haas

   • edited description
   • changed status to closed

   • 2019-02-21T20:08:38+00:00
10. Log in to comment