Modify

Opened 2 years ago

Last modified 16 months ago

#1875 reopened enhancement

SetMask_SphericalSurface: retain mask

Reported by: cott@… Owned by: Roland Haas
Priority: unset Milestone:
Component: Other Version: development version
Keywords: Cc:

Description

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

Attachments (0)

Change History (8)

comment:1 Changed 2 years ago by Roland Haas

Status: newreview

comment:2 Changed 2 years ago by Roland Haas

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.

comment:3 Changed 2 years ago by Frank Löffler

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.

comment:4 Changed 2 years ago by anonymous

Owner: set to Roland Haas
Status: reviewassigned

comment:5 Changed 2 years ago by Frank Löffler

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?

comment:6 Changed 2 years ago by Frank Löffler

Status: assignedreview

comment:7 Changed 2 years ago by Roland Haas

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

comment:8 Changed 16 months ago by Frank Löffler

Status: reviewreopened

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain Roland Haas.
Next status will be 'review'.
as The resolution will be set.
to The owner will be changed from Roland Haas to the specified user.
The owner will be changed from Roland Haas to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.