#900: GRHydro: do not use the Slopelimiter function -----------------------------------+---------------------------------------- Reporter: bmundim | Owner: Type: defect | Status: new Priority: minor | Milestone: ET_2012_05 Component: EinsteinToolkit thorn | Version: Keywords: GRHydro slopelimiter | -----------------------------------+---------------------------------------- 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...
#900: GRHydro: do not use the Slopelimiter function ------------------------------------+--------------------------------------- Reporter: bmundim | Owner: Type: defect | Status: review Priority: minor | Milestone: ET_2012_05 Component: EinsteinToolkit thorn | Version: Resolution: | Keywords: GRHydro slopelimiter ------------------------------------+--------------------------------------- Changes (by bmundim):
* status: new => review
#900: GRHydro: do not use the Slopelimiter function ------------------------------------+--------------------------------------- Reporter: bmundim | Owner: Type: defect | Status: review Priority: minor | Milestone: ET_2012_05 Component: EinsteinToolkit thorn | Version: Resolution: | Keywords: GRHydro slopelimiter ------------------------------------+---------------------------------------
Comment (by jafsma@…):
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
#900: GRHydro: do not use the Slopelimiter function ------------------------------------+--------------------------------------- Reporter: bmundim | Owner: Type: defect | Status: review Priority: minor | Milestone: ET_2012_05 Component: EinsteinToolkit thorn | Version: Resolution: | Keywords: GRHydro slopelimiter ------------------------------------+---------------------------------------
Comment (by rhaas):
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).
#900: GRHydro: do not use the Slopelimiter function ------------------------------------+--------------------------------------- Reporter: bmundim | Owner: bmundim Type: defect | Status: review Priority: minor | Milestone: ET_2012_05 Component: EinsteinToolkit thorn | Version: Resolution: | Keywords: GRHydro slopelimiter ------------------------------------+--------------------------------------- Changes (by knarf):
* owner: => bmundim
#900: GRHydro: do not use the Slopelimiter function ------------------------------------+--------------------------------------- Reporter: bmundim | Owner: bmundim Type: defect | Status: review Priority: minor | Milestone: ET_2012_05 Component: EinsteinToolkit thorn | Version: Resolution: | Keywords: GRHydro slopelimiter ------------------------------------+---------------------------------------
Comment (by bmundim):
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.
#900: GRHydro: do not use the Slopelimiter function ------------------------------------+--------------------------------------- Reporter: bmundim | Owner: bmundim Type: defect | Status: review Priority: minor | Milestone: ET_2012_05 Component: EinsteinToolkit thorn | Version: Resolution: | Keywords: GRHydro slopelimiter ------------------------------------+---------------------------------------
Comment (by rhaas):
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?).
#900: GRHydro: do not use the Slopelimiter function ------------------------------------+--------------------------------------- Reporter: bmundim | Owner: bmundim Type: defect | Status: review Priority: minor | Milestone: ET_2012_05 Component: EinsteinToolkit thorn | Version: Resolution: | Keywords: GRHydro slopelimiter ------------------------------------+---------------------------------------
Comment (by bmundim):
Good point! Someone may add d and do not include in the ParamCheck. Thanks. I will include the else statement after the release.
#900: GRHydro: do not use the Slopelimiter function ------------------------------------+--------------------------------------- Reporter: bmundim | Owner: bmundim Type: defect | Status: review Priority: minor | Milestone: ET_2012_11 Component: EinsteinToolkit thorn | Version: Resolution: | Keywords: GRHydro slopelimiter ------------------------------------+--------------------------------------- Changes (by knarf):
* milestone: ET_2012_05 => ET_2012_11
#900: GRHydro: do not use the Slopelimiter function ------------------------------------+--------------------------------------- Reporter: bmundim | Owner: bmundim Type: defect | Status: closed Priority: minor | Milestone: ET_2012_11 Component: EinsteinToolkit thorn | Version: Resolution: fixed | Keywords: GRHydro slopelimiter ------------------------------------+--------------------------------------- Changes (by rhaas):
* status: review => closed * resolution: => fixed
Comment:
A patch (incl. the extra "else") was applied as part of #944.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit