backport MoL revision 225

Create issue
Issue #1633 closed
Roland Haas created an issue

Before MoL did not initialize the RHS grid functions of the slow evolved variables to zero (but GRHydro did explicitly so this never showed up).

Revision 225 adds the initialization.

Keyword: MoL
Keyword: backport

Comments (12)

  1. Frank Löffler
    • removed comment

    Why is MoLDebug used here? If this triggers, the code aborts anyway. I don't see harm in outputting too much information in that case. Also, please make the message a bit more "official". "Arg" is cute and all, but maybe you could think of something better? :)

    Could CCTK_ERROR be used instead of CCTK_WARN(0, ?

  2. Roland Haas reporter
    • removed comment

    All of these are sound suggestions. However:

    The code fragment is a copy of the lines already in Init_RHS for the regular evolved variables. Hence the names and debug output. I would rather not change names/WARN<->ERROR/etc only for the slow sector and not eg also for normal RHS, array RHS, complex RHS etc.

    As a matter of fact it would seem much more reasonable to me to rewrite all these routines that are almost identical between array evolved variables, slow evolved variables, evolved variables, complex evolved variables, complex array evolved variables (did I miss any?) such that they all use a common worker routine an a short individual wrapper. Right now MoL has very much code duplication which makes it very hard to change anything since one has to make almost the same change at many places.

  3. Frank Löffler
    • removed comment

    Your argument makes a lot of sense, and I don't want to be the one holding you back from cleaning up either. :)

  4. Ian Hinder
    • removed comment

    But not in the backport, right? The backport should just fix the wrong behaviour in as minimal a way as possible.

  5. Roland Haas reporter
    • removed comment

    I would still apply just the proposed patch to the release version. Any cleanup will happen (eventually, I was waiting for the IMEX stuff to come in but that seems stalled) in trunk.

  6. anonymous
    • removed comment

    The fix has not been backported. It is waiting for a "Please apply". This will also require some follow up patch to the slow evolved test in GRHydro though (since the current slow evolution code in MoL actually relies on the RHS not being cleared...)

  7. Ian Hinder
    • changed status to open
    • removed comment

    So if the followup patch is not implemented, the test would break? Is that just a problem with the test, or with the code? And did you mean MoL in comment 8, or GRHydro? In either case, I think we should wait until the followup patch is ready. I am changing the status to "reopened" as the followup patch is not yet available for review.

  8. Roland Haas reporter
    • removed comment

    Ah, never mind. No follow up is required for the RHS since only a different kind of multi-rate method was affected (namely the RK2-2-1 and RK4-2-1 methods but not the RK4-RK2 method used here).

  9. Log in to comment