clawpack / amrclaw

AMR version of Clawpack
http://www.clawpack.org
BSD 3-Clause "New" or "Revised" License
26 stars 45 forks source link

Allow memsize to be set in setrun.py #267

Closed rjleveque closed 3 years ago

rjleveque commented 3 years ago

If not set, the default value agrees with what was previously used, but now memsize is written to amr.data and read in by amr2.f90.

I only changed this in 2d so far, but if it looks ok I'll do the same in 1d and 3d, and also in GeoClaw, where it will be particularly useful.

It also prints out the lendim value at the end, indicating what memsize should be used in the future.

I temporarily modified examples/advection_2d_square to illustrate this. As currently set, memsize = 69200 and it doubles the size once

 Expanding storage from        69200  to       138400

and then at the end prints out:

Max usage of alloc array (can use for memsize):        69211

Running again with this value of memsize it runs without expanding alloc.

mjberger commented 3 years ago

What is the motivation for this? Why not have it done automatically the way it is now?

On Nov 1, 2020, at 18:57, Randall J. LeVeque notifications@github.com wrote:

 @rjleveque requested your review on: #267 WIP: Allow memsize to be set in setrun.py.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.

mandli commented 3 years ago

This is automatic, this is just to set the first value so it does not thrash memory immediately.

mjberger commented 3 years ago

I think the input files are becoming very complicated and we should try to stick with as few as are really needed. This one is not needed

On Nov 1, 2020, at 19:03, Kyle Mandli notifications@github.com wrote:

 This is automatic, this is just to set the first value so it does not thrash memory immediately.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.

mandli commented 3 years ago

Well that's the good thing about the python, none of the input files needs this added, it would just default anyway unless someone wanted to have this changed. The default input files probably shouldn't have this added.

rjleveque commented 3 years ago

If memsize isn't mentioned in setrun.py then it gets set to 4000000, which is also currently the default. But I run lots of problems where memsize needs to be 200 times larger and so it is doubled many times and alloc keeps getting recopied, so it seems sensible to allow setting it up front.

I thought we had discussed doing this some time ago but never got around to it.

mjberger commented 3 years ago

you should write memsize/lendim to both fort.amr and screen (write(*...)

mandli commented 3 years ago

As opposed to just putting it in AMR.data?

mjberger commented 3 years ago

I mean at the end of a run, using it as randy suggested, it''s printed out. — Marsha

On Nov 2, 2020, at 10:35 AM, Kyle Mandli notifications@github.com wrote:

As opposed to just putting it in ``` AMR.data

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/clawpack/amrclaw/pull/267#issuecomment-720546636, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGUGC46TQ2CCOZ3CEHDY3TSN3GVVANCNFSM4TG2OZUQ.

mandli commented 3 years ago

You mean then the final size of memsize and the such? I thought we had to do that anyway?

rjleveque commented 3 years ago

It was already being written to fort.amr, the last line of:

current  space usage =        48012
maximum  space usage =        68904
need space dimension =        69211

That's how I figured out what variable to print, and then I tried to give it a more informative message for the screen output. Perhaps we should give the identical message in fort.amr?

mjberger commented 3 years ago

we should give the identical message in fort.amr and on the screen. Odd that the maximum space usage is less than needed space dimension. Isn't one the max of the other?

— Marsha

On Nov 2, 2020, at 11:21 AM, Randall J. LeVeque notifications@github.com wrote:

It was already being written to fort.amr, the last line of:

current space usage = 48012 maximum space usage = 68904 need space dimension = 69211 That's how I figured out what variable to print, and then I tried to give it a more informative message for the screen output. Perhaps we should give the identical message in fort.amr?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/clawpack/amrclaw/pull/267#issuecomment-720574336, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGUGC6HFBOXYQ5I33ESIBDSN3L7DANCNFSM4TG2OZUQ.

rjleveque commented 3 years ago

@mjberger: I found that strange too but didn't dig into it. One is lenmax and the other lendim and I assumed you had reasons for monitoring and printing both? Is it because alloc is sometimes fragmented and so the length needed is greater than the amount used?

mandli commented 3 years ago

I actually feel like we should at some point make an effort to clear up some of our output as I have a feeling very few of us knows what actually exists everywhere.

mjberger commented 3 years ago

what you say it true. But I don't remember how it's all computed, I'd have to dig into it. IT's probably ok, since we haven't changed it in a decade.

— Marsha

On Nov 2, 2020, at 12:07 PM, Randall J. LeVeque notifications@github.com wrote:

@mjberger https://github.com/mjberger: I found that strange too but didn't dig into it. One is lenmax and the other lendim and I assumed you had reasons for monitoring and printing both? Is it because alloc is sometimes fragmented and so the length needed is greater than the amount used?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/clawpack/amrclaw/pull/267#issuecomment-720602727, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGUGC5ZHON7W74OWAGJHGLSN3RNVANCNFSM4TG2OZUQ.

rjleveque commented 3 years ago

To eliminate too much screen output, I changed it to just print

See fort.amr for more info on this run and memory usage

at the end of the run, and then also modified amr2.f90 to print something a bit more descriptive in fort.amr:

alloc array statistics:
    current alloc usage =        48012
    maximum alloc usage =        68904
required alloc memsize >=        69211

I agree with @mandli that at some point we should clean up and clarify what's being printed and add some other statistics at the end of fort.amr such as max number of patches, perhaps?

Also I wonder if we should always print all the timing info to the screen, since this is also written to timing.txt. Perhaps on the screen just print something like:

For more information on this run, timings, and memory usage, 
see the files fort.amr and timing.txt in the output directory.

But I think we should start another issue/PR for that...

mandli commented 3 years ago

Agreed.

rjleveque commented 3 years ago

I extended this to 1d and 3d. I also modified the default values of memsize so that repeated doubling gets as close to 2**30 - 1 as possible, if necessary, which is the limit of integer(kind=4) indexes.

mandli commented 3 years ago

Is this ready then to be fully tested?

rjleveque commented 3 years ago

Yes I think it's ok now. See also https://github.com/clawpack/geoclaw/pull/486.