[ET Trac] [Einstein Toolkit] #1239: AHFinderDirect uses incorrect origin_* upon recovery when horizon was never found before

[Einstein Toolkit]

#1239: AHFinderDirect uses incorrect origin_* upon recovery when horizon was never
found before
-----------------------------------------+----------------------------------
  Reporter:  reisswig@…                  |       Owner:                
      Type:  defect                      |      Status:  review        
  Priority:  major                       |   Milestone:                
 Component:  EinsteinToolkit thorn       |     Version:                
Resolution:                              |    Keywords:  AHFinderDirect
-----------------------------------------+----------------------------------

Comment (by anonymous):

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

-- 
Ticket URL: <https://trac.einsteintoolkit.org/ticket/1239#comment:3>
Einstein Toolkit <http://einsteintoolkit.org>
The Einstein Toolkit