- changed status to open
- removed comment
remove the MoL number of variables accumulators
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)
-
reporter -
- 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.
-
- 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?
-
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.
-
- removed comment
I'd say that LinearCombination is a relatively new API, and is only used by MoL. Changing it is fine.
-
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.
-
reporter - changed status to open
- removed comment
-
reporter - changed status to open
- removed comment
-
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.
-
reporter - removed comment
Erik fixed 06 in rev 214 of MoL.
-
- removed comment
Roland, I see that you change the status of this patch from "reviewed_ok" to "review". What is the reason?
-
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.
-
reporter - changed status to resolved
- removed comment
Applied as rev 215, 216 and 217 of MoL. Email send to Cactus users mailing list.
-
reporter - removed comment
-
- 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?
-
- removed comment
This is not a bug, this was a design choice. This is a large change that should not be backported.
-
- removed comment
Not all of it but there are two bugfixes one for rk86 and one for rk4-rk2 that should be backported.
-
- removed comment
There is only one for 87, and that file is broken (empty). Please apply (and backport) the 42.
-
- 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. :-)
-
reporter - removed comment
Backported fix to RK87 and RK4-RK2 as rev 218 and 219 of MoL respectively.
-
- 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.
-
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.
-
reporter - changed status to resolved
- removed comment
-
reporter - changed status to closed
- edited description
- Log in to comment