SetMask_SphericalSurface: retain mask

Issue #1875 open
Christian Ott created an issue

SetMask_SphericalSurface sets a mask based on SphericalSurface information. At every invocation, it wipes the mask, then resets it. Old behavior: mask is set from surface only if surface is active and valid. However, sometimes finding a horizon with AHFinderDirect may fail, but the mask info (i.e. the radius) may be perfectly valid. Mask is still not set in the old behavior, because AHFinderDirect makes the mask invalid (-1) if it does not find a horizon. New behavior: set mask from surface is surface is active and its minimum radius is > 0.

Pull request: https://bitbucket.org/einsteintoolkit/einsteinutils/pull-requests/1/setmask_sphericalsurface-retain-mask/diff

Pull request also adds verbose output (default: off).

Keyword:

Comments (8)

  1. Roland Haas
    • removed comment

    I added a handful of comments to the pull request (https://bitbucket.org/einsteintoolkit/einsteinutils/pull-requests/1/setmask_sphericalsurface-retain-mask/diff), nothing major. I would like to make sure that math.h does not need to be included.

    The patch changes setmask_sphericalsurface's from one extreme to the other: rather than always expecting the horizon to be found, it will use possibly old horizon data forever. This is probably fine until the horizon moves and some regions of the actual horizon are no longer in the mask, at which point it will likely fail in con2prim in GRhydro just as the old code did. A dangerous situation could arise when the horizon actually shrinks without moving so that more and more of the outside of the horizon is flagged as being inside.

  2. Frank Löffler
    • removed comment

    I think I used the following for the case you describe:

    AHFinderdirect::reset_horizon_after_not_finding[0] = "no"
    

    Would that also work for you (haven't tried now it this still resets the spherical surface)?

    Otherwise: it would be good to add this as a (steerable) parameter, so users can choose between the old and the new mechanism. I could see the "new" mechanics even be the default, as this is most likely what a user wants.

  3. Frank Löffler
    • removed comment

    I made a few changes, addressing Roland's comments. Roland (or any other dev): ready to go like this? I personally don't think a parameter for this is really necessary, unless there is actually a user who wants it otherwise.

    Concerning Roland's comment "rather than always expecting the horizon to be found, it will use possibly old horizon data forever": I don't see how this could happen. The thorn always initializes the mask to 'normal', and only overwrites later with 'excises' if a surface is active. If there isn't a valid surface anymore, I would expect the current code to reset everything to 'normal'. Am I missing something?

  4. Roland Haas
    • removed comment

    The current code will do what you describe. The proposed code would use the last found surface information (since it ignores sf_valid which AHFinderDirect sets). AHFinderDirect behaves like this (src/driver/BH_diagnostics.cc)

      // only try to copy AH info if we've found AHs at this time level
      if (! AH_data.search_flag) {
        sf_valid[surface_number] = 0;
        return;
      }
      // did we actually *find* this horizon?
      if (! AH_data.found_flag) {
        sf_valid[surface_number] = -1;
        return;
      }
      sf_valid        [surface_number] = 1;
    

    ie it will set sf_valid to -1 if an AH was looked for but not found, so that current code that only accepts surfaces for which sf_valid >= 0 will reject the surface. One may instead possibly use sf_active[] && sf_valid[] which should works about as well as the proposed change though it may differ in the time interval before the AH is found for the first time (but is already searched for).

  5. Log in to comment