Refactor testing code #41
Conversation
|
This PR is ready, @lsoucasse could you have a look and let me know what you think? |
There was a problem hiding this comment.
Hi @stefsmeets, thanks for this work.
I really like the shape of the new test scripts that are much more readable. Actually I think the get_spectrum_data and the get_atmosphere_config functions should be part of the Janus module rather than test helpers.
But my main question is about the number of values to be tested. I get your point in reducing the time spent in testing but even if it is the same function that is called, it potentially addresses different coupling convection-radiation regimes. What we could do is testing three orbital values for the instellation test (extremes and middle) and five values in the runaway greenhouse test making sure we capture the flux plateau followed by its increase. Maybe @timlichtenberg or @nichollsh you could share some physics guidelines for this?
|
Capturing the runaway plateau is the key behaviour in the runaway test. Physically, this is checking that the temperature profile sits on the saturation curve for H2O, and therefore covers the physics relating to latent heat and the temperature profile integrator. It also tests SOCRATES longwave calculations. The instellation test captures the surface energy balance stuff relating to conduction through a solid skin layer above the magma ocean. This also tests the SOCRATES longwave and shortwave calculations. We can probably restrict this to only a few points, as @lsoucasse suggests, as long as they are well placed (i.e. extrema and the plateau in the runaway case, and high/low instellation in the other case). |
|
Can you suggest other ways to reduce the runtime for this test, e.g. by reducing the resolution or otherwise? In my opinion, it is still on the long side, especially if there is a need to test multiple starting values. I'm mostly interested in ensuring that the code gives the expected output. What in the model or code could change that it would give the expected output at the lower extrema, but not at the higher one? Then we should directly test that instead. |
|
The runaway plateau is – unfortunately for this issue – an emergent behaviour that results from a combination of T-P structure, opacity data, and radiative transfer. You need the whole code to achieve these numbers and not a single function can do this. This test is therefore somewhat an integrated test of the overall physical behaviour of the code. Lower resolutions have been previously shown to change the value we aim at here (which is an accepted literature value). So I do not have immediate ideas how to simplify/speed up this test. |
|
Okay, it's fairly easy to parametrize the tests now, so I can set it up in a way that new points can be easily added. It may also be worth having a flag to trigger the full integration test, so it is less disruptive when coding. |
|
I have parametrized the tests, so make it easier to add/remove points. I have added the min/max inputs and a middle input to capture the flux plateau. Still slow, but at least a factor 4 improvement over the original code. Let me know what you think. |
There was a problem hiding this comment.
Looks good,
Parametrization gives flexibility in choosing the number of points we want to test.
I suggest we comment the extra reference values instead of deleting them so that we store this information and can easily perform a comprehensive test locally.
For some reason I cannot run the tests on my machine, it gets stuck in the first one and do not move further on. But the tests run fine through the github action.
Co-authored-by: Laurent Soucasse <[email protected]>
Co-authored-by: Laurent Soucasse <[email protected]>
* Add fwl_data to .gitignore * Clean test files * Move env check to conftest * Avoid updating class variable * Clean paths and use tmp directory for output * Avoid running the same tests twice in a PR * Refactos greenhous test * Refactor instellation test * Clean imports and run code in tmpdir * Set absolute path for FWL_DATA * Fix download * Parametrize tests * Limit number of tests * Update tests/test_runaway_greenhouse.py Co-authored-by: Laurent Soucasse <[email protected]> * Update tests/test_instellation.py Co-authored-by: Laurent Soucasse <[email protected]> --------- Co-authored-by: Laurent Soucasse <[email protected]>
Hi all, to familiarize myself a bit with the code I had a look at refactoring the test cases. I reduced the number of cycles, so that the tests run about 4 times faster (<2 min instead of 7 min on my pc). I also cleaned and refactored the code, so that the tests now share most of the setup. And the tests now run in a temporary directory, which avoids cluttering the local directory.
Addresses #31