Add pytest-based tests and remove nose tests. #721
Conversation
|
This works well for me. I am onboard with the idea of making the tests more verbose and also adding accuracy tests where we can. What would be even better would be convergence but I imagine that being a bit tricky. |
|
I also agree it would be best to make the tests more verbose and easier to understand and modify. I wonder about running convergence tests, since part of the point of the tests is to make them run very quickly so that we do it frequently, and so they can be done automatically for PRs. Testing convergence is of course important, but once it's been determined that a particular test behaves properly in this regard, isn't it enough to test a few things coming out of one quick run to make sure the example hasn't broken in some way due to changes elsewhere? I thought that was the point of these unit tests, not to validate the numerical method. We should talk about how to make similar changes in other repos, e.g. updating clawpack/amrclaw#279. A couple other thoughts regarding the Fortran code tests:
|
|
Thanks for the comments! In the meantime, I did figure out how to setup similar tests with different function arguments in a more streamlined way: https://docs.pytest.org/en/7.1.x/example/parametrize.html But maybe it's better to just be verbose and explicit. We can discuss it later today. |
* Add test for nonunif advection and remove nose from advection * Add tests for 1d advection with variable coeffs Testing for some custom RK time integrators, relies on dictionaries. * rephrasing comment * Add tests for 2D advection Testing transverse solver. I think there is too much numerical dissipation with such a coarse grid to compare initial conditions with solution at final time. Using a dictionary with expected solutions instead. * Add test advection_annulus * Update test for advection 1D with nonuniform grid Using dictionary with expected solution instead of comparison between initial condition and final solution. * Remove unnecessary abs * Update docstring * Rename classes for consistency * Add test Burgers 1d * Fix bug relative path * Update tests Euler 2D Switch to pytest and use a single test file. I am not using the tools from pyclaw.utils. Should we test them with the examples? If so, maybe we should keep them in one or two examples. I am skipping tests for shock_forward_step, so the HLLE RS with walls is not tested. This example gives me some random NaNs that I am not able to handle robustly. * Add tests Euler 3D Add tests for the other 2 examples for 3D Euler. * Do not abort execution if there is no MPI * Add init file for relative imports to work Is there a reason to not treat this directory as a package? * Better to not treat this one as package for the moment * Update test Euler with gravity 3d I didn't see why the test (with nose) was skipped if scipy couldn't be found. I removed that requirement here. The execution of both examples is not aborted now if mpi4py is not available, just a warning is displayed. The same .txt file with data for the regression test is used. The local Makefile is used to compile the source with subprocess. * Redirect output Sedov Euler 3d * Skipping PeanoClaw tests for the moment * Handle relative path for Pytest * Fix import * Disable output when testing * Update test psystem 2d Updated to work with pytest and removed dependence on pyclaw.util. * Remove nosetest Euler w gravity 3D * Add pytest SWEs 1D, remove nosetests * Redirect output and remove nose calls * Fix test class name * Add pytests SWEs 2D * Remove nose call * Add pytests stegoton 1d * Passing some paremeters as arguments for setup * Add pytests traffic 1D * Remove nose call * Add expected solutions Burgers 1D --------- Co-authored-by: Carlos Munoz Moncayo <munozmca@kl-26147.kaust.edu.sa> Co-authored-by: Carlos Munoz Moncayo <munozmca@kl-26147.local>
ketch
changed the title
|
I think this is ready to go, thanks mostly to @carlosmunozmoncayo . Any additional comments from @mandli @rjleveque are welcome. |
|
Thanks @ketch and @carlosmunozmoncayo , you've done a lot of work on this! Many of the tests pass for me, but the one in and the one in |
There was a problem hiding this comment.
Running pytest from the root catches the tests in development. Instead I ran pytest src examples.
Output:
➜ pyclaw git:(add_pytests) ✗ pytest examples src
============================= test session starts ==============================
platform darwin -- Python 3.11.9, pytest-7.4.3, pluggy-1.3.0
rootdir: /Users/kmandli/Documents/src/clawpack
plugins: anyio-4.0.0
collected 67 items
examples/acoustics_1d_homogeneous/test_acoustics.py ...... [ 8%]
examples/acoustics_2d_homogeneous/test_2d_acoustics.py .... [ 14%]
examples/acoustics_2d_mapped/test_acoustics_2d_mapped.py . [ 16%]
examples/acoustics_2d_variable/test_acoustics_2d_variable.py ... [ 20%]
examples/acoustics_2d_variable/test_acoustics_2d_variable_io.py . [ 22%]
examples/acoustics_3d_variable/test_3d_acoustics.py .. [ 25%]
examples/advection_1d/test_advection.py ...... [ 34%]
examples/advection_1d/test_advection_nonunif.py .. [ 37%]
examples/advection_1d_variable/test_variable_coefficient_advection.py .. [ 40%]
... [ 44%]
examples/advection_2d/test_2d_advection.py ..... [ 52%]
examples/advection_2d_annulus/test_2d_advection_annulus.py . [ 53%]
examples/advection_reaction_2d/test_advection_reaction.py . [ 55%]
examples/burgers_1d/test_burgers_1d.py .... [ 61%]
examples/cubic_1d/test_cubic.py . [ 62%]
examples/euler_1d/test_shocksine.py . [ 64%]
examples/euler_1d/test_shocktube.py . [ 65%]
examples/euler_1d/test_woodward_colella_blast.py . [ 67%]
examples/euler_2d/test_euler2d.py ... [ 71%]
examples/euler_3d/test_euler3d.py ..s [ 76%]
examples/euler_gravity_3d/test_eulerg3d.py F [ 77%]
examples/mhd_1d/test_shocktube.py . [ 79%]
examples/peano_shallow_2d/test_identical_grids.py s [ 80%]
examples/peano_shallow_2d/test_peano_solver.py s [ 82%]
examples/psystem_2d/test_2d_psystem.py . [ 83%]
examples/shallow_1d/test_shallow1d.py .. [ 86%]
examples/shallow_2d/test_shallow2d.py .. [ 89%]
examples/shallow_sphere/test_shallow_sphere.py . [ 91%]
examples/stegoton_1d/test_stego.py .... [ 97%]
examples/traffic/test_traffic.py .. [100%]
The failure in test_eulerg3d.py looks to be due to a missing import for mappedGrid coming from rising_hot_sphere.py, which does have the line
from mappedGrid import euler3d_mappedgrid as mg
The problem here of course is that mappedGrid is compiled. Once I did that it worked. Was this supposed to be built at installation? The broader point I am bringing up though is that without guessing that I needed to go in and build the module I am not sure how to have known to do this. Not sure that the test should try and do this but some information might be useful. This will also be important for CI testing.
Also, while I was looking at the Makefile in there I noticed that the path to the riemann repository was relative rather than using CLAW. Was there a reason to do this? Although certainly an uncommon occurrence It seems unsafe to assume relative paths between the sub-repositories.
|
Thank you very much for your feedback! @rjleveque the test @mandli |
|
Yes, it would be good to make the 3D euler example be compiled and installed at run-time. @carlosmunozmoncayo if you want to do that, that would be great. I suppose it should be a separate pull request that could be merged before this one. @mandli We don't require |
I saw that, not suer why it did not compile for me. May be unrelated.
I did vaguely recall that but I am wondering if this is a good idea. I had not really thought about this but the placement of other repositories relative to PyClaw seems like it could break. We could set the variable by default to be relative so the user would not need to do this usually but then one could stick Riemann or something else anywhere if they were so inclined. Not sure though as such as user would also more than likely be savvy enough to see the path and know to change it. 🤷 |
|
@mandli I agree it's worth thinking about. From my perspective, adding a requirement to set an environment variable is a significant additional barrier to some users. So far I've never received a report from someone having an issue with this relative path. It seems like a very rare corner case since they would have to move repositories around and try to run this specific example and not know enough to look at the path in the makefile and correct it. |
|
@ketch Any change should not cause any additional requirements for sure. I was thinking that something like a |
|
@mandli Oh, you're right; I hadn't understood what you meant. Defaulting to using |
|
@rjleveque @mandli I've merged some other PRs with changes relative to your comments. I'd like to also go ahead and merge this, given how much it improves the state of pyclaw testing, even if some issues may remain. Then it would be great if you could run the tests again and open issues for any failures. |
|
Agreed. |
Since nose is dead, this PR begins the switch to using pytest in PyClaw. Some notes: