Update and improve Riemann solver code #176
Conversation
It uses PyClaw's tests since, to my understanding, an ensemble of local regression tests for the Riemann repository using Pytest would require the appropriate argument intents of the rp* subroutines to be explicitly declared already. So the Riemann solvers should be modernized first. Also, the test for shallow water on the sphere is omitted for the moment.
The PyClaw test that uses this RS is skipped in the testing workflow since it leads to a core dump (I suspect this has to do with the OS or the system architecture). However, this test passes locally for me (before and after the implicit none changes).
The regression tests in AMRClaw are not being collected by Pytest. They just have to be renamed. This is a temporary fix to run them from this repo in a single pytest session.
Update branch, add testing workflow, use implicit none in more Riemann solvers
|
This PR is now ready for consideration to merge and I've requested reviews from @mandli and @rjleveque . Most of the work was done by @carlosmunozmoncayo . Here are more details:
There are two currently failing tests, but both of them fail prior to this PR:
|
There was a problem hiding this comment.
Mostly just some general stylistic comments. The testing needs to be fixed and I would suggest that we change all mentions of double precision to real(kind=8). I was also thinking that we could trigger only specific tests from the other packages, including GeoClaw so that it take more time and is more directed.
|
|
||
| on: | ||
| push: | ||
| branches: [ "implicit_none" ] |
There was a problem hiding this comment.
This won't work in general and is specific to only this branch/PR
There was a problem hiding this comment.
Ah yes, that was just a hack to get it to test this branch for the moment.
| @@ -0,0 +1,19 @@ | |||
| #Collect Pytest tests for AMRClaw | |||
There was a problem hiding this comment.
I am confused by this script. Does the workflow above for AMRClaw not work?
There was a problem hiding this comment.
If I try pytest ${CLAW}/amrclaw, Pytest does not collect the regression tests in AMRClaw. I think it tries to match the pattern *_test.py and skips regression_tests.py. This script was just a temporary way to collect the tests and pass them explicitly to Pytest. Probably the correct thing to do would be to rename the tests in the AMRClaw repo and not merge this script. I can open a PR there for that.
There was a problem hiding this comment.
Modifications were required to get the collection to work as you mentioned. This is hopefully going to fix that. Not sure what to do until then.
| dimension auxr(maux, 1-mbc:maxm+mbc) | ||
| !Input | ||
| integer, intent(in) :: maxm, meqn, mwaves, maux, mbc, mx | ||
| double precision, intent(in) :: ql(meqn, 1-mbc:maxm+mbc) |
There was a problem hiding this comment.
Might suggest that we consistently use real(kind=8) rather than double precision or if we are being really crazy implement the ability to have variable precision with something like real(kind=RPP).
| @@ -70,7 +76,7 @@ subroutine rp1(maxm,meqn,mwaves,maux,mbc,mx,ql,qr,auxl,auxr,fwave,s,amdq,apdq) | |||
| fwave(2,2,i) = -beta2*zz | |||
| s(2,i) = cc | |||
|
|
|||
| 20 END DO | |||
| END DO | |||
| amdq(m,i) = fwave(m,1,i) | ||
| apdq(m,i) = fwave(m,2,i) | ||
| 220 END DO | ||
| end do | ||
| END DO | ||
|
|
||
| return |
There was a problem hiding this comment.
Do not need return at the end of the subroutines.
| @@ -156,7 +164,10 @@ double precision function sigma(eps,i,coefA,coefB) | |||
| ! -------------------------------------------- | |||
| double precision function sigmap(eps,i,coefA,coefB) | |||
| implicit none | ||
|
|
||
| real(kind=8) :: eps, coefA, coefB | ||
| integer :: i |
There was a problem hiding this comment.
May want to explicitly say that sigmap is the return variable.
| @@ -182,4 +193,4 @@ double precision function sigmap(eps,i,coefA,coefB) | |||
|
|
|||
|
|
|||
| return | |||
ketch
changed the title
This PR is intended to improve the existing code without changing any functionality.
It is far from complete. Before merging it, I also want to get all the tests and continuous integration working in PyClaw.