Skip to content
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

Add downstream tests #226

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Add downstream tests #226

merged 1 commit into from
Sep 26, 2024

Conversation

charleskawczynski
Copy link
Member

This PR adds some downstream tests.

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.40%. Comparing base (06416e9) to head (a1f2667).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #226      +/-   ##
==========================================
- Coverage   90.93%   90.40%   -0.53%     
==========================================
  Files          10        9       -1     
  Lines        1180     1167      -13     
==========================================
- Hits         1073     1055      -18     
- Misses        107      112       +5     
Flag Coverage Δ
90.40% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@szy21 szy21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Should we add one from from ClimaAtmos examples too, or is the test good enough?

@charleskawczynski
Copy link
Member Author

Should we add one from from ClimaAtmos examples too, or is the test good enough?

Ideally, the Atmos unit tests should give a good signal to whether the examples work or not. So, if a change here does not break the Atmos unit tests but does break examples, then the Atmos tests need to be expanded on.

Otherwise, where do we stop? Do we also add the long runs? This is like what @sriharshakandala was talking about. Please confirm, @Sbozzolo.

@charleskawczynski charleskawczynski added the Launch Buildkite Add label to launch Buildkite label Sep 25, 2024
@charleskawczynski charleskawczynski added this pull request to the merge queue Sep 25, 2024
@Sbozzolo Sbozzolo removed this pull request from the merge queue due to a manual request Sep 25, 2024
@Sbozzolo
Copy link
Member

Sbozzolo commented Sep 25, 2024

Should we add one from from ClimaAtmos examples too, or is the test good enough?

Ideally, the Atmos unit tests should give a good signal to whether the examples work or not. So, if a change here does not break the Atmos unit tests but does break examples, then the Atmos tests need to be expanded on.

I absolutetly agree with this.

ClimaTimesteppers and ClimaDiagnostics don't depend on Thermodynamics, so you can remove them.

There is an additional coupler test you can add to run a few steps of AMIP. It will probably fail right now because SST and SIC artifacts became too big that cannot be downloaded automatically. You can start by downloading those files automatically (as we do here), but I will also produce a smaller version that can be downloaded automatically in the future.

@charleskawczynski
Copy link
Member Author

There is an additional coupler test you can add to run a few steps of AMIP. It will probably fail right now because SST and SIC artifacts became too big that cannot be downloaded automatically. You can start by downloading those files automatically (as we do here), but I will also produce a smaller version that can be downloaded automatically in the future.

IMO, we should stick to the unit tests, which should have a tightly coupled signal to the experiments, and encourage/support a culture of thorough unit tests.

@Sbozzolo
Copy link
Member

There is an additional coupler test you can add to run a few steps of AMIP. It will probably fail right now because SST and SIC artifacts became too big that cannot be downloaded automatically. You can start by downloading those files automatically (as we do here), but I will also produce a smaller version that can be downloaded automatically in the future.

IMO, we should stick to the unit tests, which should have a tightly coupled signal to the experiments, and encourage/support a culture of thorough unit tests.

ClimaCoupler is very different: unit tests in ClimaCoupler are only for the coupling infrastructure but have no physics in it (with the coupler intentially left as a thin and agnostic package). All the physics is in the ClimaEarth experiment, which runs AMIP. Turning ClimaEarth into its own package is part of our longer term project goals, but that is not currently being worked on.

@charleskawczynski
Copy link
Member Author

ClimaCoupler is very different: unit tests in ClimaCoupler are only for the coupling infrastructure but have no physics in it (with the coupler intentially left as a thin and agnostic package).

There are physics-related pieces in the ClimaCoupler test suite. For example, here's a function with physics currently in the test suite:

function FieldExchanger.update_sim!(sim::TestAtmos, fields, _)
    (; F_turb_ρτxz, F_turb_energy, F_turb_moisture) = fields
    ρ_int = sim.integrator.ρ
    @. sim.integrator.p.energy_bc = -(F_turb_energy)
    @. sim.integrator.p.ρq_tot_bc = -F_turb_moisture
    @. sim.integrator.p.uₕ_bc = -(F_turb_ρτxz / ρ_int) # x-compoennt only for this test
end

All the physics is in the ClimaEarth experiment, which runs AMIP. Turning ClimaEarth into its own package is part of our longer term project goals, but that is not currently being worked on.

If something in the ClimaEarth experiment relies on some sort of physics, that should also be included in the unit test suite. In fact, if it's only exercising components from Atmos, then we should catch that even earlier, in the Atmos unit tests. I would rather us be consistent about this, than make exceptions that people don't understand and are subject to change (first its AMIP, next its CMIP). As @sriharshakandala said during our discussion, running all upstream experiments is not scalable, and I agree. Our unit tests should have a good signal that the experiments will run, if they don't then the unit tests should be expanded on.

@charleskawczynski charleskawczynski added this pull request to the merge queue Sep 26, 2024
Merged via the queue into main with commit c919fd7 Sep 26, 2024
25 of 26 checks passed
@Sbozzolo
Copy link
Member

Sbozzolo commented Sep 26, 2024

Consider that the coupler team is composed by 1 person at the moment. ClimaEarth is currently living inside ClimaCoupler because there is no person-power to work on it and make it a separate package with its unit tests. This is also because it was decided that this is not a priority for our project right now.

I absolutely agree with you that unit tests should be enough and we should expand on them, but if you don't include the ClimaEarth test I can tell you that you are missing an important piece that is important to us.

@Sbozzolo
Copy link
Member

Sbozzolo commented Sep 26, 2024 via email

@charleskawczynski
Copy link
Member Author

I absolutely agree with you that unit tests should be enough and we should expand on them, but if you don't include the ClimaEarth test I can tell you that you are missing an important piece that is important to us.

Which pieces? I can help add them to the coupler unit test

@juliasloan25
Copy link
Member

Perhaps the root of the issue comes from the fact that we've developed the coupler so far with component model-agnostic coupling infrastructure in src/, and some component model-specific adapter code in experiments/ClimaEarth/ - for example extensions of getter and setter functions we need to update fluxes. The long term goal is to move all the model-specific code in experiments/ClimaEarth/ into the respective component repositories, but that isn't the state we're at currently.

The ClimaCoupler unit tests do cover our source code, e.g. interface functions, flux calculations, etc, but they don't cover the component model-specific experiment code. This is why a short downstream AMIP experiment is helpful to inform us if e.g. any interfaces have broken. I agree that we should have thorough unit tests for all of our code, but ClimaCoupler is in a tricky situation right now and as Gabriele said it will require some considerable work to get it to where we want it to be.

Also, I'm not sure what is meant by adding a unit test for the AMIP experiment - would this look like running a coupled simulation for a couple of timesteps, or something different?

@charleskawczynski @Sbozzolo

@charleskawczynski
Copy link
Member Author

Ok, fair point about considerable remaining work. I'm fine with adding this test temporarily. Is there a relatively future-proof way to add it to the downstream tests here?

@juliasloan25
Copy link
Member

Ok, fair point about considerable remaining work. I'm fine with adding this test temporarily. Is there a relatively future-proof way to add it to the downstream tests here?

I think the best way is to add the same test Atmos is using (added here). It's been in the test suite there for a few months and as far as I know it hasn't caused problems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Launch Buildkite Add label to launch Buildkite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants