- changed status to open
- removed comment
GRHydro: do not use the Slopelimiter function
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 (9)
-
reporter -
- 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
-
repo owner - 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).
-
- assigned issue to
- removed comment
-
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.
-
repo owner - 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?).
-
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.
-
- changed milestone to ET_2012_11
- removed comment
-
repo owner - changed status to resolved
- removed comment
A patch (incl. the extra "else") was applied as part of
#944. - Log in to comment