Empty ApplyBCs call in LeanBSSNMoL

Issue #2648 resolved
Samuel Cupp created an issue

In the schedule.ccl of LeanBSSNMoL starting at line 55, the following ApplyBCs call is scheduled:

schedule GROUP ApplyBCs as LeanBSSN_ApplyBCs at CCTK_INITIAL after LeanBSSN_adm2bssn
{
} "Apply boundary conditions"

However, no boundary selection routines are scheduled before it. As far as I am aware, this is a do-nothing call. Should the function LeanBSSN_Boundaries be scheduled before this?

Comments (13)

  1. Samuel Cupp reporter

    This also appears in Baikal and BaikalVacuum, which may have copied this behavior from Lean.

  2. Zach Etienne

    I can confirm that I copied most of Lean’s scheduling when creating Baikal*. It would be nice to know the reasoning behind the scheduling of this group in Lean. :)

  3. Roland Haas

    The scheduled functions in the ApplyBCs group will not do anything unless there has been a SelectGroupForBCs first though (and no intervening ApplyBCs which clears the selections). SelectGroupForBCs does not have to be immediatedly before the ApplyBCs group though, so there could be a "forgotten" / "hidden" one somewhere earlier in the code. Also if this is during initial data only then this may not be very visible at all if the initial data thorn did fill in data everywhere on all grid points irrespective of them being ghosts, buffer, physical boundary, symmetry boundary or interior points. After ADM2BSSN one usually has to SYNC, extrapolate and apply symmetry boundary conditions (possibly in a very specific order) and the Gamma auxiliary variables of the BSSN system which involve derivatives so cannot be set in ghost zones and the boundary. Most others can be computed point by point, so that a missing SYNC (missing for sure it seems) will only show up a slight change in regions filled via prolongation (but not in inter-processor ghosts or in restriction target region).

  4. Roland Haas

    Note: “major” bugs are those that affect results and must be fixed before the next release. If the maintainers of lean_public and Baikal could look into this and adjust the severity setting accordingly that would be great.

  5. helvi witek

    So, for full disclosure let me copy here the email conversion with Sam, Miguel and myself (apologies for email formatting):

    It sounds like you would want something like LeanBSSN_Boundaries running before that. However, looking at that function I see quite a lot of grid functions in it. Do you want all of them to run? If you only want gammat you'll (unfortunately) probably need another function with just it. However, it's probably fine to apply symmetry conditions to the other grid functions (gconf_fac, hmetric, hcurv, trk, lapse, shift). Again, I wouldn't support that if it was in the evolution loop, but a few extra symmetry BCs in initial shouldn't really matter for speed unless its a very short run.

    Also, keep in mind that I haven't actually put in any prints inside CartGrid3D and am going purely off of looking at the code. You may want to see if anything is actually being applied before changing the code or scheduling. I just thought it looked incorrect and thought you might want to check it out.

    I had started a ticket a while back, so we can use that if there's any changes you want to make or questions for the maintainers.

    Samuel Cupp    Postdoctoral Researcher
       Department of Physics    University of Idaho


    From: Miguel Zilhão <mzilhao@ua.pt>
    Sent: Saturday, January 21, 2023 3:05 AM
    To: Cupp, Samuel D. <scupp1@my.apsu.edu>; Helvi Witek <hwitek@illinois.edu>
    Subject: Re: LeanBSSNMoL scheduling  

    hi Sam,

    thanks for your detailed reply. i understand better your point and i'm now a bit confused. let me
    elaborate below.

    Thanks for the quick response. The ghost zones (inter-processor boundaries) wouldn't be triggered by
    this. They are triggered by

    schedule LeanBSSN_adm2bssn at CCTK_INITIAL after ADMBase_PostInitial
    {
        LANG: Fortran
        OPTIONS: Local
        SYNC: gammat
    } "Convert initial data into BSSN variables"

    where the *SYNC* statement is.

    indeed, you're totally right.

    I asked Steve and Roland about when symmetry BCs are applied just to
    make sure I understood correctly, and the symmetry BCs are applied whenever the physical BCs are
    applied. However, physical BCs only get applied by the ApplyBCs function when a Select*ForBC
    function call has actually registered a function with the Boundary thorn. Since no Select*ForBC
    functions are scheduled before this ApplyBC call, it shouldn't do anything.
    ah, wait. my understanding was that we were registering the symmetry BCs in the following call in
    the schedule file:

    schedule LeanBSSN_symmetries at BASEGRID
    {
       LANG: Fortran
       OPTIONS: Global
    } "Register symmetries of the BSSN grid functions"

    called at "BASEGRID". inside the routine "LeanBSSN_symmetries" we have

    call SetCartSymVN( ierr, cctkGH, (/ 1, 1, 1/), "LeanBSSNMoL::conf_fac" )
       call SetCartSymVN( ierr, cctkGH, (/ 1, 1, 1/), "LeanBSSNMoL::rhs_conf_fac" )

    call SetCartSymVN( ierr, cctkGH, (/ 1, 1, 1/), "LeanBSSNMoL::hxx" )
       call SetCartSymVN( ierr, cctkGH, (/-1,-1, 1/), "LeanBSSNMoL::hxy" )
       call SetCartSymVN( ierr, cctkGH, (/-1, 1,-1/), "LeanBSSNMoL::hxz" )
       call SetCartSymVN( ierr, cctkGH, (/ 1, 1, 1/), "LeanBSSNMoL::hyy" )
       call SetCartSymVN( ierr, cctkGH, (/ 1,-1,-1/), "LeanBSSNMoL::hyz" )
       call SetCartSymVN( ierr, cctkGH, (/ 1, 1, 1/), "LeanBSSNMoL::hzz" )

    (...)

    so what you're saying is that calling "ApplyBCs" after this is meaningless by itself unless one
    actually calls the Select*ForBC functions before? indeed, it seems we only call Select*ForBC much
    later (at MoL_PostStep).

    Roland mentioned that
    "Correct. If there is not SelectBCs call then nothing will happen. It *is* possible that some of the
    old-style symmetry boundary conditions from CartGrid3D still get applied, which has some code in
    there (I think) to work with thorn Boundary."

    it could be that there is something making sure that some symmetry conditions are applied, otherwise
    in every run with symmetry the gammat* grid functions would have non-initialized values at the
    symmetry points, which would likely create problems...

    So I looked to see how CartGrid3D triggers its BCs, and it also relies on the selected variables
    from Boundary. So, best I can tell this does nothing. It isn't a real issue or anything, as its a
    few empty function calls that happens once at INITIAL. Removing it wouldn't make the code faster or
    anything, but I don't think it is needed. Alternatively, you may want a function SelectionVarForBC
    that actually causes gammat to have symmetry BCs applied.

    i think this is probably the safest thing, just adding a line in the schedule file to make sure the
    "LeanBSSN_Boundaries" function is also called after at CCTK_INITIAL. what do you think?

    thanks,
    Miguel


    *From:* Miguel Zilhão <mzilhao@ua.pt>
    *Sent:* Friday, January 20, 2023 2:58 AM
    *To:* Helvi Witek <hwitek@illinois.edu>; Cupp, Samuel D. <scupp1@my.apsu.edu>
    *Subject:* Re: LeanBSSNMoL scheduling
    hi Sam and Helvi,

    if i remember correctly, that call to "ApplyBCs" is there since in the routine
    LeanBSSN_adm2bssn we're not setting the values of the grid functions
    gammat{x,y,z} at the ghostzones (since this is done via some derivatives). we
    then call "ExtrapolateGammas" on these functions to set the values at the outer
    boundary, but the inter-processor and symmetry boundaries will remain unset.

    with the call to "ApplyBCs", inter-processor and symmetry boundaries are
    applied, and the gammat* grid functions are then defined everywhere.

    this was the reasoning, i think. does it make sense?

    cheers,
    Miguel

    On Thu, 2023-01-19 at 13:25 -0600, Helvi Witek wrote:

    Dear Sam,
    let me also cc Miguel.

    As far as I recall, this boundary scheduling is the same as for McLachlan. It
    is done right after the conversion from adm to bssn (including computing the
    conformal connection function).

    @Miguel, do you recall the details of the "ApplyBCs" call here?

    Best wishes,
    Helvi


    Dr. Helvi Witek
    Assistant Professor
    Department of Physics
    University of Illinois at Urbana-Champaign

    247 Loomis Lab
    1110 W Green St
    Urbana, IL 61801


    On 1/19/23 5:49 PM, Cupp, Samuel D. wrote:

    Hey Helvi,
        I'm wondering if maybe my previous email went to your junk. A while ago
    I had asked about LeanBSSNMoL. I'll just copy the previous email into this
    one.

    I have been working on Baikal and BaikalVacuum (in wvuthorns), and they
    copied a lot of their scheduling from LeanBSSNMoL. In doing so, I noticed a
    weird scheduled group in the schedule.ccl that is also in LeanBSSNMoL. I was
    hoping you or someone else who worked on Lean could explain why it is there.

    In LeanBSSNMoL's schedule.ccl is

    schedule GROUP ApplyBCs as LeanBSSN_ApplyBCs at CCTK_INITIAL after LeanBSSN_
    adm2bssn
    {
    } "Apply boundary conditions"

    However, there are no boundary condition selection routines which run
    before this, and nothing is scheduled relative to this function. As best I
    can tell, this should do nothing and could just be removed. Is there
    something I'm missing? Or, should there be a SelectBCs routine running here
    that is missing? Thanks for your help.

    Samuel Cupp     Postdoctoral Researcher
        Department of Physics

  6. helvi witek

    And the conversation between Roland and myself:
    hwitek  3:40 PM
    actually Sam had contacted me directly via email, but yes this should be in a ticket.

    3:43

    The original reason was to apply inner (refinement or ghost zone) boundary condition with the Cactus/Carpet “ApplyBCs” function, directly after computing the bssn variables, including $\Gamma^{i}$.
    It is not clear to me that “ApplyBCs” really does nothing, and am therefore hesitant to just take it out without it having been tested for a non-trivial case.

    Roland Haas  9:39 AM

    so many avenues of interaction. The scheduled functions in the ApplyBCs group will not do anything unless there has been a SelectGroupForBCs first though (and no intervening ApplyBCs which clears the selections). SelectGroupForBCs does not have to be immediatedly before the ApplyBCs group though, so there could be a "forgotten" / "hidden" one somewhere earlier in the code. Also if this is during initial data only (and I really should do as I say and use the ticket)... then this may not be very visible at all if the initial data thorn did fill in data everywhere on all grid points irrespective of them being ghosts, buffer, physical boundary, symmetry boundary or interior points.

  7. helvi witek

    Regarding this being a “major bug:

    Note: “major” bugs are those that affect results and must be fixed before the next release. If the maintainers of lean_public and Baikal could look into this and adjust the severity setting accordingly that would be great.

    Since the physics results have been thoroughly tested and benchmarked, I do not evaluate as a major bug. If Sam is correct, it would do nothing.
    However, this needs to be checked for a non-trivial example (e.g. binary black holes), before we take it out.

    @Samuel Cupp @Miguel Zilhão Could you please run a set of 2 tests, one with and one without the “ApplyBCs” being called right after adm2bssn? I suggest to used the standard, small BBH run with q=1, d=6M

  8. helvi witek

    After ADM2BSSN one usually has to SYNC, extrapolate and apply symmetry boundary conditions (possibly in a very specific order) and the Gamma auxiliary variables of the BSSN system which involve derivatives so cannot be set in ghost zones and the boundary. Most others can be computed point by point, so that a missing SYNC (missing for sure it seems) will only show up a slight change in regions filled via prolongation (but not in inter-processor ghosts or in restriction target region).

    @Roland: Ok, Let me add the relevant part of schedule.ccl:

    schedule LeanBSSN_adm2bssn at CCTK_INITIAL after ADMBase_PostInitial
    {
      LANG: Fortran
      OPTIONS: Local
       SYNC: gammat
    } "Convert initial data into BSSN variables"
    
    schedule GROUP ApplyBCs as LeanBSSN_ApplyBCs at CCTK_INITIAL after LeanBSSN_adm2bssn
    {
    } "Apply boundary conditions"
    

    In Lean, we extrapolate gammat at the end of the routine “LeanBSSN_adm2bssn”, and sync as shown.

    Our understanding was that “ApplyBCs” then applies the symmetry boundary conditions.

  9. Miguel Zilhão

    Please see the following commit in the development branch: https://bitbucket.org/canuda/lean_public/commits/87ec2413906abc8c925be51d78ecc4a2e8e194d3. It makes sure that SelectGroupForBCs are calles before ApplyBCs at CCTK_INITIAL; here’s the relevant diff:

    -schedule GROUP ApplyBCs as LeanBSSN_ApplyBCs at CCTK_INITIAL after LeanBSSN_adm2bssn
    +schedule LeanBSSN_Boundaries at CCTK_INITIAL after LeanBSSN_adm2bssn
    +{
    +  LANG: Fortran
    +  OPTIONS: LEVEL
    +  SYNC: LeanBSSNMoL::gammat
    +} "Boundary enforcement"
    +
    +schedule GROUP ApplyBCs as LeanBSSN_ApplyBCs at CCTK_INITIAL after LeanBSSN_Boundaries
    

    Would this fix the issue?

  10. Samuel Cupp reporter

    You should only sync in one of either LeanBSSN_adm2bssn or LeanBSSN_Boundaries but not both, which would be the behavior with this change. If you want to be consistent with your later scheduling, I think you put the sync on the Boundaries function and not the adm2bssn, but that’s ultimately up to you.

  11. Log in to comment