clawpack / pyclaw

PyClaw is a Python-based interface to the algorithms of Clawpack and SharpClaw. It also contains the PetClaw package, which adds parallelism through PETSc.
http://www.clawpack.com/pyclaw
BSD 3-Clause "New" or "Revised" License
157 stars 98 forks source link

Add pytest-based tests and remove nose tests. #721

Closed ketch closed 2 months ago

ketch commented 5 months ago

Since nose is dead, this PR begins the switch to using pytest in PyClaw. Some notes:

mandli commented 5 months ago

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.

rjleveque commented 5 months ago

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 https://github.com/clawpack/amrclaw/pull/279.

A couple other thoughts regarding the Fortran code tests:

ketch commented 5 months ago

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.

ketch commented 3 months ago

I think this is ready to go, thanks mostly to @carlosmunozmoncayo . Any additional comments from @mandli @rjleveque are welcome.

rjleveque commented 3 months ago

Thanks @ketch and @carlosmunozmoncayo , you've done a lot of work on this!

Many of the tests pass for me, but the one in examples/shallow_2d/radial_dam_break.py fails with:

E               AssertionError: Test radialdambreak_sharpclaw_hlle failed
E               assert 1.9122676246041693e-06 < 1e-06
E                +  where 1.9122676246041693e-06 = error(test_name='radialdambreak_sharpclaw_hlle', solver_type='sharpclaw', riemann_solver='hlle', disable_output=True)
test_shallow2d.py:32: AssertionError

and the one in shallow_sphere/test_shallow_sphere.py dies with a segmentation fault,

(claw510) [shallow_sphere] niwot $ pytest -v .
============================= test session starts ==============================
platform darwin -- Python 3.10.13, pytest-8.0.1, pluggy-1.4.0 -- /Users/rjl/venv/claw510/bin/python3.10
cachedir: .pytest_cache
rootdir: /Users/rjl/Downloads/clawpack-v5.10.0pyclaw/pyclaw/examples/shallow_sphere
collected 1 item                                                               

test_shallow_sphere.py::test_shallow_sphere Fatal Python error: Segmentation fault

Current thread 0x00000001052b8580 (most recent call first):
  File "/Users/rjl/Downloads/clawpack-v5.10.0pyclaw/pyclaw/examples/shallow_sphere/Rossby_wave.py", line 444 in setup
  File "/Users/rjl/Downloads/clawpack-v5.10.0pyclaw/pyclaw/src/pyclaw/util.py", line 235 in test_app
  File "/Users/rjl/Downloads/clawpack-v5.10.0pyclaw/pyclaw/examples/shallow_sphere/test_shallow_sphere.py", line 23 in test_shallow_sphere
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/python.py", line 194 in pytest_pyfunc_call
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_callers.py", line 102 in _multicall
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_manager.py", line 119 in _hookexec
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_hooks.py", line 501 in __call__
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/python.py", line 1831 in runtest
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/runner.py", line 174 in pytest_runtest_call
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_callers.py", line 102 in _multicall
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_manager.py", line 119 in _hookexec
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_hooks.py", line 501 in __call__
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/runner.py", line 267 in <lambda>
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/runner.py", line 346 in from_call
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/runner.py", line 266 in call_runtest_hook
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/runner.py", line 227 in call_and_report
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/runner.py", line 134 in runtestprotocol
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/runner.py", line 115 in pytest_runtest_protocol
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_callers.py", line 102 in _multicall
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_manager.py", line 119 in _hookexec
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_hooks.py", line 501 in __call__
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/main.py", line 352 in pytest_runtestloop
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_callers.py", line 102 in _multicall
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_manager.py", line 119 in _hookexec
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_hooks.py", line 501 in __call__
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/main.py", line 327 in _main
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/main.py", line 273 in wrap_session
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/main.py", line 320 in pytest_cmdline_main
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_callers.py", line 102 in _multicall
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_manager.py", line 119 in _hookexec
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/pluggy/_hooks.py", line 501 in __call__
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/config/__init__.py", line 175 in main
  File "/Users/rjl/venv/claw510/lib/python3.10/site-packages/_pytest/config/__init__.py", line 198 in console_main
  File "/Users/rjl/venv/claw510/bin/pytest", line 8 in <module>

Extension modules: numpy.core._multiarray_umath, numpy.core._multiarray_tests, numpy.linalg._umath_linalg, numpy.fft._pocketfft_internal, numpy.random._common, numpy.random.bit_generator, numpy.random._bounded_integers, numpy.random._mt19937, numpy.random.mtrand, numpy.random._philox, numpy.random._pcg64, numpy.random._sfc64, numpy.random._generator, clawpack.riemann.acoustics_1D, clawpack.riemann.acoustics_variable_1D, clawpack.riemann.acoustics_1D_ptwise, clawpack.riemann.advection_1D, clawpack.riemann.advection_1D_ptwise, clawpack.riemann.advection_color_1D, clawpack.riemann.burgers_1D, clawpack.riemann.cubic_1D, clawpack.riemann.euler_with_efix_1D, clawpack.riemann.euler_hlle_1D, clawpack.riemann.mhd_roe_1D, clawpack.riemann.nonlinear_elasticity_fwave_1D, clawpack.riemann.psystem_fwave_1D, clawpack.riemann.reactive_euler_with_efix_1D, clawpack.riemann.shallow_hlle_1D, clawpack.riemann.shallow_roe_with_efix_1D, clawpack.riemann.shallow_bathymetry_fwave_1D, clawpack.riemann.shallow_roe_tracer_1D, clawpack.riemann.traffic_1D, clawpack.riemann.traffic_vc_1D, clawpack.riemann.traffic_vc_fwave_1D, clawpack.riemann.traffic_vc_tracer_1D, clawpack.riemann.acoustics_2D, clawpack.riemann.acoustics_mapped_2D, clawpack.riemann.acoustics_2D_ptwise, clawpack.riemann.advection_2D, clawpack.riemann.burgers_2D, clawpack.riemann.euler_mapgrid_2D, clawpack.riemann.euler_5wave_2D, clawpack.riemann.euler_4wave_2D, clawpack.riemann.euler_hlle_2D, clawpack.riemann.euler_hlle_with_walls_2D, clawpack.riemann.kpp_2D, clawpack.riemann.psystem_2D, clawpack.riemann.shallow_hlle_2D, clawpack.riemann.shallow_roe_with_efix_2D, clawpack.riemann.shallow_bathymetry_fwave_2D, clawpack.riemann.sw_aug_2D, clawpack.riemann.shallow_sphere_2D, clawpack.riemann.vc_acoustics_2D, clawpack.riemann.vc_advection_2D, clawpack.riemann.vc_elasticity_2D, clawpack.riemann.vc_acoustics_3D, clawpack.riemann.euler_3D, clawpack.riemann.burgers_3D, clawpack.riemann.vc_advection_3D, matplotlib._c_internal_utils, PIL._imaging, matplotlib._path, kiwisolver._cext, matplotlib._image, clawpack.pyclaw.examples.shallow_sphere.sw_sphere_problem, clawpack.pyclaw.classic.classic2_sw_sphere (total: 66)
Segmentation fault: 11
carlosmunozmoncayo commented 3 months ago

Thank you very much for your feedback!

@rjleveque the test examples/shallow_2d/radial_dam_break.py also failed for me on a different machine. I realized that the previous tolerance was larger. I will set it back to the original value. I have not been able to reproduce the error with examples/shallow_sphere/test_shallow_sphere.py. In this PR, only a call to nose was removed from that test. Could the segmentation fault error be related to an issue with the compilation of sw_sphere_problem or classic2_sw_sphere at installation time?

@mandli mappedGrid and euler_3d_gmap are not built at installation. test_eulerg3d.py uses a subprocess to compile them before importing rising_hot_sphere. However, the subprocess uses the Makefile which, as you mention, relies on a relative path to the riemann repository to look for the sources. Since these modules are only called from the files in pyclaw/examples/euler_gravity_3d, we could move the source there and compile at installation, similarly to what is done for examples/shallow_sphere and examples/advection_reaction_2d.

ketch commented 3 months ago

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 CLAW to be set for PyClaw installation or usage.

mandli commented 3 months ago

mappedGrid and euler_3d_gmap are not built at installation. test_eulerg3d.py uses a subprocess to compile them before importing rising_hot_sphere. However, the subprocess uses the Makefile which, as you mention, relies on a relative path to the riemann repository to look for the sources. Since these modules are only called from the files in pyclaw/examples/euler_gravity_3d, we could move the source there and compile at installation, similarly to what is done for examples/shallow_sphere and examples/advection_reaction_2d.

I saw that, not suer why it did not compile for me. May be unrelated.

We don't require CLAW to be set for PyClaw installation or usage.

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

ketch commented 3 months ago

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

mandli commented 3 months ago

@ketch Any change should not cause any additional requirements for sure. I was thinking that something like a make variable that can be over ridden if not defined to do the relative path would work but it's not that big of a deal.

ketch commented 3 months ago

@mandli Oh, you're right; I hadn't understood what you meant. Defaulting to using CLAW if it is set is a good idea.

ketch commented 2 months ago

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

mandli commented 2 months ago

Agreed.