WeylScal4 schedule order is incorrect

Create issue
Issue #1171 closed
Ian Hinder created an issue

WeylScal4 contains a calculation for computing the I and J invariants from the Psis. This calculation is not scheduled explicitly after the Psis. Therefore, Cactus will probably schedule the routines alphabetically, which should be wrong. This should be corrected either with an After or Schedule element in the calculation.

Keyword: WeylScal4
Keyword: testsuite
Keyword: backport

Comments (13)

  1. Erik Schnetter
    • changed status to open
    • removed comment

    In addition to correcting the schedule, the enclosed patch also adds a "WeylScal4_" prefix to all schedule items, and evaluates the invariants pointwise instead of applying boundary conditions.

    Index: WeylScal4.m
    --- WeylScal4.m (revision 124)
    +++ WeylScal4.m (working copy)
    @@ -302,7 +302,7 @@
     Psi4Calc[fdOrder_, PD_] :=
    -  Name -> "psi4_calc_" <> fdOrder,
    +  Name -> "WeylScal4_psi4_calc_" <> fdOrder,
       Where -> Interior,
       After -> "ADMBase_SetADMVars",
       ConditionalOnKeywords -> {{"fd_order", fdOrder}, {"calc_scalars", "psi4"}},
    @@ -312,7 +312,7 @@
     PsisCalc[fdOrder_, PD_] :=
    -  Name -> "psis_calc_" <> fdOrder,
    +  Name -> "WeylScal4_psis_calc_" <> fdOrder,
       Where -> Interior,
       After -> "ADMBase_SetADMVars",
       ConditionalOnKeywords -> {{"fd_order", fdOrder}, {"calc_scalars", "psis"}},
    @@ -322,9 +322,9 @@
     InvariantsCalc[fdOrder_, PD_] :=
    -  Name -> "invars_calc_" <> fdOrder,
    -  Where -> Interior,
    -  After -> "ADMBase_SetADMVars",
    +  Name -> "WeylScal4_invars_calc_" <> fdOrder,
    +  Where -> Everywhere,
    +  After -> "WeylScal4_psis_calc_" <> fdOrder <> "_group",
       ConditionalOnKeywords -> {{"fd_order", fdOrder}, {"calc_scalars", "psis"}, {"calc_invariants", "always"}},
       Equations -> invariantEqs

    We should also modify one of the test cases to calculate the invariants as well.

  2. Ian Hinder reporter
    • changed status to open
    • removed comment

    I think this is a candidate for backporting to the release. My only concern is that computing the invariants pointwise rather than using boundary conditions will change numerical results, and is an enhancement not a fix. Additionally, the change in name of the functions could cause other thorns which rely on these names for scheduling purposes to break. I would backport the fix to the scheduling order only. What do others think?

  3. Roland Haas
    • removed comment

    Can we separate out the bugfix from the ehancement? Ideally we would also not re-release until the tests pass on at least some of the machines (some that are convenient or important I would guess). We don't want to accidentally break the release branch.

  4. Ian Hinder reporter
    • removed comment

    I have set up automated tests for the release branch (https://build.barrywardell.net/job/EinsteinToolkitReleased/). These are only run on a single machine (a ubuntu VM), not a production machine. The tests will only be run AFTER we commit to the release branch. And at that point, the code will be downloaded by anyone checking out the released version, as we tell people to check out by branch not by tag. I have moved this discussion to the mailing list, as it affects a wider audience.

  5. Ian Hinder reporter
    • removed comment

    For the upcoming release, we should fix the schedule order only. The pointwise computation and the renaming both break backward compatibility.

  6. Frank Löffler
    • removed comment

    Isn't this patch already applied, and thus, also the point-wise calculation? It seems that this bug was only re-opened to get the schedule-change into the 2012_11 release, which by now is kind of pointless. Can this bug be closed again?

  7. Log in to comment