GRHydro: do not use the Slopelimiter function

Create issue
Issue #900 closed
Bruno Mundim created an issue

This patch was generated by Josh Faber in response to the discussion at http://lists.einsteintoolkit.org/pipermail/users/2012-January/001710.html but it wasn't ever applied. It essentially deprecates the use of slopelimiter function. In the future it should be completely removed from the code. At the moment it is only commented out.

Ok to apply? it is a bug fix...

Keyword: GRHydro
Keyword: slopelimiter

Comments (10)

  1. Joshua Faber
    • removed comment

    To avoid spurious limiters being allowed, we also need to

    1.) eliminate the parameter options minmod2, minmod3, and vanleermc1 for GRHydro::tvd_limiter 2.) Get rid of the file GRHydro_SlopeLimiter.F90 3.) Eliminate the variables minmod2, minmod3, mc1 from GRHydro_Scalars.F90

  2. Roland Haas
    • removed comment

    Reading the email archive: the bugfix is in SUPERBEE, the other changes simply remove limiters which we suspect to be wrong but don't care enough about to actually fix, yes?

    The patch seems to not have a final "else" clause anymore which is generally a bad idea (since one should never assume no one will ever add a case "d" to the cases "a", "b", "c" we are testing for). The removed slopelimiter had such a line:

    call CCTK_WARN(0, "Type of limiter not recognized")

    Beyond that the consensus seems to have been to apply this patch. So: please apply (possibly adding an "else" clause).

  3. Bruno Mundim reporter
    • removed comment

    Reading the email archive: the bugfix is in SUPERBEE, the other changes simply remove limiters which we suspect to be wrong but don't care enough about to actually fix, yes?

    Not quite. If I remember correctly vanleerMC was actually an implementation of minmod.

    The patch seems to not have a final "else" clause anymore which is generally a bad idea (since one should never assume no one will ever add a case "d" to the cases "a", "b", "c" we are testing for). The removed slopelimiter had such a line:

    Isn't the else clause essentially already checked at GRHydro_ParamCheck.F90? If we type a tvd_limiter that is not in the list at param.ccl, the code is aborted.

    In any case, I added the simple logic to abort if using the deprecated values of tvd_limiters. Please let me know if it is ok to apply.

  4. Roland Haas
    • removed comment

    The problem with relying on ParamCheck is what happens when a new slope limiter is introduced in param. ccl (not very likely admittedly). Without an aborting "else" case, the new option would be silently ignored. ParamCheck will easily catch removed options but will not help you (the programmer) if you would like to add a new option. I see it analogous to always having a "default:" case in a switch statement even if all it does is abort (with maybe the possible exception if the same test was done in the same routine close by before. But even then, what happens if this piece of code is copied or lot's of other code added in between?).

  5. Bruno Mundim reporter
    • removed comment

    Good point! Someone may add d and do not include in the ParamCheck. Thanks. I will include the else statement after the release.

  6. Log in to comment