[Einstein Toolkit] #1505: remove the MoL number of variables accumulators

List overview All Threads
Download

newer

older

[Einstein Toolkit] #1560:...

[Einstein Toolkit] #1559: Replace...

Einstein Toolkit

13 Dec 2013 13 Dec '13

12:49 p.m.

#1505: remove the MoL number of variables accumulators -----------------------------------+---------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: new Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Keywords: MoL | -----------------------------------+---------------------------------------- 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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Show replies by date

Einstein Toolkit

13 Dec 13 Dec

12:50 p.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: review Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: MoL ------------------------------------+--------------------------------------- Changes (by rhaas):

* status: new => review

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:1 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

1:46 p.m.

Comment (by sbrandt):

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:2 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

11 Feb 11 Feb

9:28 p.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: reviewed_ok Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: MoL ------------------------------------+--------------------------------------- Changes (by eschnett):

* status: review => reviewed_ok

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?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:3 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

12 Feb 12 Feb

4:14 p.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: reviewed_ok Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: MoL ------------------------------------+---------------------------------------

Comment (by rhaas):

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:4 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

5:03 p.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: reviewed_ok Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: MoL ------------------------------------+---------------------------------------

Comment (by eschnett):

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

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:5 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

14 Feb 14 Feb

4:24 a.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: reviewed_ok Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: MoL ------------------------------------+---------------------------------------

Comment (by rhaas):

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

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:6 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

17 Feb 17 Feb

10:42 a.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: reopened Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: MoL ------------------------------------+--------------------------------------- Changes (by rhaas):

* status: reviewed_ok => reopened

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:7 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

10:42 a.m.

* status: reopened => review

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:8 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

10 Mar 10 Mar

7:50 a.m.

Comment (by rhaas):

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

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:9 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

6:27 p.m.

Comment (by rhaas):

Erik fixed 06 in rev 214 of MoL.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:10 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

11 Mar 11 Mar

11:05 p.m.

Comment (by eschnett):

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

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:11 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

12 Mar 12 Mar

12:01 a.m.

Comment (by rhaas):

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:12 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

12:54 a.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: closed Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: fixed | Keywords: MoL ------------------------------------+--------------------------------------- Changes (by rhaas):

* status: review => closed * resolution: => fixed

Comment:

Applied as rev 215, 216 and 217 of MoL. Email send to Cactus users mailing list.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:13 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

13 Mar 13 Mar

11:37 a.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: closed Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: fixed | Keywords: MoL backport ------------------------------------+--------------------------------------- Changes (by rhaas):

* keywords: MoL => MoL backport

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:14 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

11:44 a.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: closed Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: fixed | Keywords: MoL backport ------------------------------------+---------------------------------------

Comment (by knarf):

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?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:15 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

11:47 a.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: closed Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: fixed | Keywords: MoL backport ------------------------------------+---------------------------------------

Comment (by eschnett):

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

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:16 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

11:52 a.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: closed Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: fixed | Keywords: MoL backport ------------------------------------+---------------------------------------

Comment (by anonymous):

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

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:17 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

11:53 a.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: closed Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: fixed | Keywords: MoL backport ------------------------------------+---------------------------------------

Comment (by knarf):

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

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:18 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

11:58 a.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: closed Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: fixed | Keywords: MoL backport ------------------------------------+---------------------------------------

Comment (by anonymous):

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

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:19 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

6:14 p.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: closed Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: fixed | Keywords: MoL backport ------------------------------------+---------------------------------------

Comment (by rhaas):

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

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:20 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

15 Mar 15 Mar

9:01 p.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: reopened Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: MoL backport ------------------------------------+--------------------------------------- Changes (by barry.wardell):

* status: closed => reopened * resolution: fixed =>

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:21 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

11:05 p.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: reopened Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: MoL backport ------------------------------------+---------------------------------------

Comment (by rhaas):

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:22 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

11:06 p.m.

#1505: remove the MoL number of variables accumulators ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: closed Priority: optional | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: fixed | Keywords: MoL backport ------------------------------------+--------------------------------------- Changes (by rhaas):

* status: reopened => closed * resolution: => fixed

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1505#comment:23 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

4590

Age (days ago)

4683

Last active (days ago)

trac@lists.einsteintoolkit.org

23 comments

1 participants

tags (0)

participants (1)

Einstein Toolkit