GRHydro: do not use the Slopelimiter function

Comments (10)

1. 

   Bruno Mundim reporter

   • changed status to open
   • removed comment

   • 2012-05-16T10:20:48+00:00
2. 

   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

   • 2012-05-18T15:03:00+00:00
3. 

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

   • 2012-05-20T15:16:05+00:00
4. 

   Frank Löffler

   • assigned issue to
   • removed comment

   • 2012-05-21T10:32:40+00:00
5. 

   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.

   • 2012-05-21T17:22:53+00:00
6. 

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

   • 2012-05-21T17:29:13+00:00
7. 

   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.

   • 2012-05-21T17:36:56+00:00
8. 

   Frank Löffler

   • changed milestone to ET_2012_11
   • removed comment

   • 2012-05-28T10:27:47+00:00
9. 

   Roland Haas

   • changed status to resolved
   • removed comment

   A patch (incl. the extra "else") was applied as part of #944.

   • 2012-06-05T15:53:47+00:00
10. 

    Roland Haas

    • edited description
    • changed status to closed

    • 2019-02-21T20:19:41+00:00
11. Log in to comment