remove the MoL number of variables accumulators

Issue #1505 closed
Roland Haas created an issue

The attached patches removes the need for MoL's accumulator parameters to count the number of evolved, constrained and save-and-restore variables.

Instead it counts them during MoL_RegisterVariables and using timelevels as a way of creating new variable storage for the scratch levels. This means unfortunately that it will create C pointers for 99 (the current max in interface.ccl) scratch timelevels ie there is ScratchSpace_p_p_p_p..._p .

Currently applies on top of trunk.

Keyword: MoL
Keyword: backport

Comments (24)

  1. Steven R. Brandt
    • removed comment

    Roland's patch is far more detailed than mine, and sets the array size to CCTK_NumVars(), instead of calling realloc. This is clearly going to be safe and is simpler.

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

    Patch 01 looks good.

    Patch 02 looks good.

    mol.patch looks almost fine: - There is a stray "printf" that probably wants to go away. - It contains malloc/realloc calls that may look nicer in C++ (std::vector!), or that could be simplified in C: Instead of

    EvolvedVariableIndex = (CCTK_INT *)malloc(sizeof(CCTK_INT)*newSize);
    

    write

    EvolvedVariableIndex = malloc(newsize * sizeof *EvolvedVariableIndex);
    

    (no need to cast the malloc return value; no need to remember the type of EvolvedVariableIndex). That's mostly a question of preference, of course.

    Patch 03: - Why is LinearCombination renamed? Aliased functions are type-safe, so all callers will notice. - Consider using CCTK_VError for errors (not important).

    With these caveats: Please apply, then announce on mailing list.

    Question: Should the accumulator parameter be removed, or at least its documentation changed and marked obsolete? Maybe output a paramcheck warning if it is set?

  3. Roland Haas reporter
    • removed comment

    I had to read through the patches myself to see what is going on. mol.patch looks like Steve's original patch from #1435. It was not actually up for review, sorry.

    Is it possible to add a way to remove attachments to trac?

    Patch 03: Reasons for renaming were that MoL uses LinearCombination but it does not provide it. The modified MoL needs a different functionality than what is provided by LinearCombination hence it uses a different function LinearCombination2 which it also does not provide. If we are happy changing LinearCombination's interface without having to go through the deprecate-announce-remove cycle of two ET releases then I am fine modifying its interface. I'd deprecate the parameter and remove two releases from now.

  4. Erik Schnetter
    • removed comment

    I'd say that LinearCombination is a relatively new API, and is only used by MoL. Changing it is fine.

  5. Roland Haas reporter
    • removed comment

    I updated all patches. Fixes bugs in the patches and in MoL. Wrote test thorn to test (most of) MoL's ODE methods.

  6. Roland Haas reporter
    • removed comment

    Applied patch 01 and 02 as rev 212 and 213 of MoL. Updated patch 03, 05, 06 to apply after rev 210.

  7. Erik Schnetter
    • removed comment

    Roland, I see that you change the status of this patch from "reviewed_ok" to "review". What is the reason?

  8. Roland Haas reporter
    • removed comment

    Erik, good question. I think I had not realized the "with these caveats, please apply". I had since then added the bugfix-commits 05 and 06 and the new thorn TestMoL. I suspect I also change to review again since the MoL.patch file was never intended for review. Anyhow, I'll apply the current stack of patches and send an email to the list. I am still working on changes to the drivers and the flesh to avoid having to specify 99 time levels for the MoL variables.

  9. Frank Löffler
    • removed comment

    Do we need to backport this? I was under the impression that having to specify the number of levels is more an inconvenience, not really an important bug?

  10. Erik Schnetter
    • removed comment

    This is not a bug, this was a design choice. This is a large change that should not be backported.

  11. anonymous
    • removed comment

    Not all of it but there are two bugfixes one for rk86 and one for rk4-rk2 that should be backported.

  12. Frank Löffler
    • removed comment

    There is only one for 87, and that file is broken (empty). Please apply (and backport) the 42.

  13. anonymous
    • removed comment

    Erik made a separate patch for 87 and applied it. So I wiped the 87 in this ticket. I'll apply and backport 42 and backport 87. I take this to have been a review of the 42 patch. :-)

  14. Roland Haas reporter
    • removed comment

    Backported fix to RK87 and RK4-RK2 as rev 218 and 219 of MoL respectively.

  15. Barry Wardell
    • changed status to open
    • removed comment

    I've just noticed (thanks to warnings from gcc) that patch 03 (which has been committed as rev 215) is missing quotation marks at the end of lines 3028 and 3152 and the start of lines 3029 and 3153.

  16. Roland Haas reporter
    • removed comment

    Replying to [comment:21 barry.wardell]:

    I've just noticed (thanks to warnings from gcc) that patch 03 (which has been committed as rev 215) is missing quotation marks at the end of lines 3028 and 3152 and the start of lines 3029 and 3153.

    Applied correction as rev 220 of MoL.

  17. Log in to comment