-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to rpn3, rpt3, and rptt3_euler.f90 #75
Conversation
Updated 3D Euler equations for q-wave for use with dimensional splitting. Adapted from current rpn3_euler.f90.
Removed authorship info from comments
put "Adapted from..." line back in. Removed it by accident on last commit
deleting old rpn3_euler.f90
renaming to replace older rpn3_euler.f90
fixed small bug in rpn3 with size of auxl and auxr arrays.
Modified for Fortran 90/95 styling, similar to how rpn3_euler.f90 was modified.
Modified for Fortran 90/95 styling, and fixed a small bug in IF statement location. See Issue clawpack#74 (clawpack#74)
stop | ||
endif | ||
|
||
IF (maxmrp < maxm+2*mbc)THEN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this can be slower but I would favor the removal of a fixed overhead with maxmrp
and just use the correct amount of memory via maxm+2*mbc
(which is also not optimal).
Thanks for taking the time to do this @weslowrie. One comment I had is about our general use of a maximum work array dimension within the Riemann solver (indicated on the source), it is very possible that not doing this would cause a significant decrease in performance (compilers yet again prove too dumb). |
It looks like just some of the Roe averaged quantities use this for a vector size. @mandli I am fine with your suggestion, as I am not very familiar with the impact of the work array size. Would you suggest just having: |
That's what I was thinking although as I mentioned, I have never done any performance tests to see how much this effects everything. My naive guess is that this does not matter in the grand scheme of things but it would be wise to check. |
I'll try your suggestion @mandli to see if I notice a difference. I'm also going to guess that it will not make much of a difference. |
@weslowrie This is meant to replace #73, right? |
@mandli , @weslowrie I played around with switching to dynamic allocation of those arrays for the Sedov problem at 64^3 resolution. I did three runs with each version, and the dynamic allocation version was slower by 6-7%. I would also prefer to avoid hard-coded array sizes, but that's a noticeable penalty. I'm using gfortran/f2py, probably with no optimization flags (though I don't remember how to control those in f2py). |
By the way, the new 3D Euler tests in PyClaw pass with this PR, so we can merge it once we decide what to do with the maxmrp parameter. |
Maybe we should table the debate about |
I am surprised that this is leading to this strong of a penalty, ideally (and this is probably the problem) the compiler should recognize the automatic arrays for what they are and not reallocate them every entry to the function. This is probably not an issue for the parameter set array size. This is of course exactly the role of a work array, something we could also do more easily in a module and typedef setting. In any case, I agree with @ketch given @ketch, @weslowrie and my own experience and that we should probably just table this for the "future". |
Okay, I raised an issue for the work arrays (#76). I'm merging this. Very nice work, @weslowrie. |
Update to rpn3, rpt3, and rptt3_euler.f90
Modified rpt3 and rptt3 for Fortran 90/95 styling.
Fixed a small bug in rpn3_euler.f90, and another small bug in rptt3_euler.f90 (See Issue #74 #74)