-
Notifications
You must be signed in to change notification settings - Fork 37
More plots for the RBC paper #594
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
Conversation
| assert not np.allclose(NuI, ref_data['Nu']['V']) | ||
| assert np.allclose(tI, ref_data['t']) | ||
|
|
||
|
|
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.
Why are there no decorators here to flag the tests?
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.
The project tests are all run in the project specific environment. So in the current CI, environment markers for project tests have no impact.
The GPU project is tested on JUWELS using the environment we set up there, so it's not even using micromamba.
Now that I think about it, it may make sense to put the 3D RBC stuff in its own project so the tests can be run on GitHub and CPUs. For testing purposes, CPUs should be fine.
What do you think? New project or tests on JUWELS?
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.
Both, since we need the GPU tests?
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 don't think we really need GPU tests for this. We have tests that the GPU port of spectral discretizations generally works, so CPU only testing for this should be fine. While a test of course doesn't hurt, I think the work involved here would not be worth while.
I'll set up a new project then and do CPU tests.
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.
OK! What should we do with this PR, 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 suggest we merge it and then I do the move in a separate PR to keep the diff small.
This PR includes a plot for the spectrum and one for the order of accuracy for various simulation schemes. Functions that compute stuff are tested. Functions that only plot stuff or that only supply parameters for plotting are excluded from the coverage report.