-
Notifications
You must be signed in to change notification settings - Fork 44
ThermodynamicSeaIce testing #814
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
@pierluigividale there are some issues when using the Test Summary: | Pass Fail Total Time
Ocean and sea ice models | 24 4 28 4.3s
OceanModel = SeasonalOceanClimatology | 5 2 7 1.3s
SeaIceModel = ThermodynamicSeaIce | 1 2 3 0.8s
SeaIceModel = NoSeaIce | 4 4 0.5s
OceanModel = ConstantOceanClimatology | 5 2 7 1.0s
SeaIceModel = ThermodynamicSeaIce | 1 2 3 0.5s
SeaIceModel = NoSeaIce | 4 4 0.5s
OceanModel = AquaPlanet | 7 7 0.9s
OceanModel = SlabOcean | 7 7 1.0s That's why we need tests 😉 |
OK, this is definitively the way to go when developing a model, but what exactly do these Pass/Fail mean? |
Okay, so I've added an if mask[ij] < 1 && isfinite(sst[ij]) # at least partially ocean, SST not NaN (=masked) So we now have the behaviour that if SST is NaN then don't change sea ice concentration. If it's 0 it stays zero, if it's 1 it'll stay that too. That would allow you to define an ice-up snowball planet in terms of albedo even though no SSTs are defined or something, anyway, it's the easiest solution so I'm happy to stick with that. If people want NaNs then they get weird edge cases 🤣 |
I've been writing these tests SpeedyWeather.jl/test/physics/ocean_sea_ice.jl Lines 26 to 33 in 55c3a9d
testing for all combinations of ocean and sea ice models. Sea ice concentration should stay within 0 and 1, albedo too and for no sea ice it should be no sea ice. Tests run for only a few days as everything else feels like an overkill for continuous integration. As I said we can add at some point long integrations but normal CI is not the point for it because I don't want to wait for 4 hours before I know whether I can merge a pull request or not |
Your example is precisely what I was looking for: how did you find out that this was the issue? A more general question, I think that I have seen examples on how to change continents, that is, to go back to serious paleoclimates, but how do people generate things like SSTs then? Is it all done by hand, or is there a BC generation tool? |
Tests pass locally though now 🎉 |
OK, that helps me to understand. So, when you write a new piece of code you can also define its own testset, which is then used for a large number of different experiments. Nice, particularly as one adds complexity and it is hard to anticipate all possible interactions of different bits of code. I was lucky with my climate setup, as I avoided this particular trap, but in a few weeks I may have tried to do an ocean denial experiment, then this would have blown up in my face. Cheers, |
Well if you don't put NaNs in your SSTs then you could just redefine the land sea mask on the fly without troubles. If something was land then turns ocean it would start with the initial condition of whatever value is in that grid cell. Hence I like the idea to define SSTs actually everywhere to something reasonable. It's the job of the mask to mask things not of the SSTs. |
If you want to learn how we test the code looks into the test folder. The script runtests.jl just includes all files that test specific parts/components. Each file then uses Julia's unit testing explained here https://docs.julialang.org/en/v1/stdlib/Test/ In the end you need to formulate lines starting with @test followed by something that is supposed to return true. If it doesn't it's flagged as fail. Every test you add is run for every commit we push to the repository over and over again. This sounds like an overkill but really saves you if your code has some unintended and potentially dangerous side effects. It would also be an overkill to run long simulations as tests but we usually stick to short ones. Hence we want to test whether the user interface works, model construction doesn't fail and that no errors or blowups happen on the first time steps. The tests also record if errors happen outside of @test lines. So a test set is already useful even if it doesn't contain any @test line! |
Awesome tests and documentation passes now. Before we merge something with documentation we quickly check that one too. A preview of the documentation for this pull request is found here https://speedyweather.github.io/SpeedyWeatherDocumentation/previews/PR814/ocean/ I wrote a section on ocean and sea ice models with some maths and how to use them. Would you have a look and then you can merge if you're happy with the docs too? |
I have read through and it is quite clear.
I do have a suggestion on ordering of sections for ocean, and I have found some typos on SeaIce.
Would you like for me to list them, or is there a way for me to edit directly?
Cheers,
PL
… On 13 Aug 2025, at 15:08, Milan Klöwer ***@***.***> wrote:
Would you have a look and then you can merge if you're happy with the docs too?
|
Fixed a couple of typos
Happy for you to make the changes directly to the markdown files in |
I have changed the order of the options, on what I perceive to be a complexity axis. My notion of what this is may be incorrect, but I intended to go from simplest to most complex. Also, in the SlabOcean energy balance you have listed the sensible heat flux as S, but it is more common to use H, as S is normally dedicated to Salinity. Would it hurt anything to change to H for sensible heat flux?
Eventually we can also trigger an ocean+sea ice simulation to run inside the documentation. Everything inside the |
OK, I went ahead and did that without waiting for your message. Not as dangerous as M&S on untested code, I am starting to behave myself.
Please have a look at what I have done with the ordering of the ocean options in ocean.md
Also see my comment about sensible heat flux.
Cheers,
PL
… On 13 Aug 2025, at 17:14, Milan Klöwer ***@***.***> wrote:
Happy for you to make the changes directly to the markdown files in /docs/src every commit will retrigger building of the docs and you can look at the preview with the same link once there's a green tick on "documentation" in CI (plus a minute or so to deploy)
|
Right, just received a scary message saying that all documentation checks have failed. Lovely. But I see that checks are running again. I do need to run to the shop, so can only look at this later tonight. |
He says and still breaks the documentation, fixed it for you though 😉 The code in
blocks has to be read from top to bottom, across all those blocks that have the same scope determined by |
Also I don't think I explained this. Whenever a check fails (tests or documentation or buildkite whatever) you can click on the red cross and it directs you to the error message thrown in the tests or the documentation. So in most cases you get a understandable explanation of whats currently wrong! So for the documentation failing you can go up, click on the commit/the red cross and you'll see that the documentation fails because of ┌ Error: failed to run `@example` block in docs/src/ocean.md:23-25
│ ```@example ocean
│ ocean = AquaPlanet(spectral_grid)
│ ```
│ exception =
│ UndefVarError: `spectral_grid` not defined in `Main.__atexample__named__ocean`
│ Suggestion: check for spelling errors or missing imports. (sometimes you have to scroll a bit to find the exact place that threw the error, not all errors make the documentation fail, it runs through line by line and tries to keep going even after the first error to list you independent problems also in one go) |
Yay now everything passes and I squash merged! |
Fantastic |
For the documentation I received one such link. For the previous tests, I was looking for one, did not spot it. That is why I was left wondering. Shall pay more attention next time around. |
* Bump version from 0.16.0 to 0.17.0 * update changelog
Followup from #811, supercedes #813