Skip to content
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

Merged
merged 8 commits into from
Jul 25, 2014
Merged

Conversation

weslowrie
Copy link
Contributor

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)

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
Copy link
Member

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

@mandli
Copy link
Member

mandli commented Jul 14, 2014

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

@weslowrie
Copy link
Contributor Author

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:
REAL(kind=8), DIMENSION(-1:maxm+mbc) :: u2v2w2,u,v,w,enth,a,g1a2,euv ?

@mandli
Copy link
Member

mandli commented Jul 16, 2014

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.

@weslowrie
Copy link
Contributor Author

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.

@ketch
Copy link
Member

ketch commented Jul 24, 2014

@weslowrie This is meant to replace #73, right?

@ketch
Copy link
Member

ketch commented Jul 24, 2014

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

@ketch
Copy link
Member

ketch commented Jul 24, 2014

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.

@ketch
Copy link
Member

ketch commented Jul 24, 2014

Maybe we should table the debate about maxmrp. When we switch to modules for the Riemann solvers, we can just allocate those arrays once.

@weslowrie
Copy link
Contributor Author

@ketch yes meant to replace #73. Sorry if there is a more efficient way to add/modify PRs.

I tried the static allocation and didn't notice much, if any difference, but did not do a thorough study. I prefer the dynamic array sizes as well.

@mandli
Copy link
Member

mandli commented Jul 24, 2014

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".

@ketch
Copy link
Member

ketch commented Jul 25, 2014

Okay, I raised an issue for the work arrays (#76). I'm merging this. Very nice work, @weslowrie.

ketch added a commit that referenced this pull request Jul 25, 2014
Update to rpn3, rpt3, and rptt3_euler.f90
@ketch ketch merged commit 81b786c into clawpack:master Jul 25, 2014
mandli added a commit to mandli/riemann that referenced this pull request Mar 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants