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

fix aquaplanet longruns #998

Merged
merged 3 commits into from
Oct 10, 2024
Merged

fix aquaplanet longruns #998

merged 3 commits into from
Oct 10, 2024

Conversation

juliasloan25
Copy link
Member

@juliasloan25 juliasloan25 commented Oct 7, 2024

Purpose

See failing longruns here

We have some longruns that evolve the atmos and surface states independently without coupling, so that we can independently test the infrastructure in the coupler that constructs and steps component models, and have a hierarchy of increasingly-complex runs. These nocouple runs are set up with a large coupling timestep (> t_end) so coupling never occurs.

This caused an error in the bucket diagnostics because the (very large) coupling timestep was being passed to the bucket model and used as its timestep. We actually have separate quantities dt and dt_cpl in our config files, where one is the component model timestep and the other is coupling timestep, but dt isn't being used except for in ClimaAtmos.get_simulation

There are a few things that should be done differently here

  • We want an option to turn off bucket diagnostics, or compute at a different frequency
  • We want each component model to use independent timesteps, so our configs should have options to specify each of them. Right now we just have dt and dt_cpl
  • The failing longrun is an "aquaplanet" experiment, which runs the ocean component over the entire surface. This is fine, but the land is actually evaluated everywhere too, and just zeroed out. I imagine this might be why bucket_init was taking in the large dt_cpl value before - it wouldn't actually ever get evaluated using such a large step. If we know that the land model isn't being used, we should be able to avoid initializing it at all. The same applies in reverse for slabplanet_terra runs, which run land over the entire surface and zero out ocean. I'm not sure what the best short-term solution is, but I think in the medium-term we should separate AMIP and slabplanet drivers

The first point is addressed in this PR, the second is partially addressed here, and the third will be addressed separately.

Content

  • add a flag to optionally output bucket diagnostics: set true by default, false for all aquaplanet runs
  • parse dt from the config dict and pass this to component models instead of dt_cpl

To-do (in separate PRs)

  • separate AMIP and slabplanet drivers; don't run land/ocean when they aren't used, instead of just masking them out
  • add options for unique dts for each component model (atmos, land, ocean, sea ice)

  • I have read and checked the items on the review checklist.

@Sbozzolo
Copy link
Member

Sbozzolo commented Oct 7, 2024

Disabling diagnostics feels like a workaround: the root cause of the problem is that we are adding a land model when there's no land. Is there any easy ways to fix that?

@juliasloan25
Copy link
Member Author

Disabling diagnostics feels like a workaround: the root cause of the problem is that we are adding a land model when there's no land. Is there any easy ways to fix that?

We can add two more branches to the big if/else here for the slabplanet_aqua and slabplanet_terra cases, but I didn't like this solution because it adds a lot of code to the driver for cases that aren't high priority. I think the solution is to create separate drivers for AMIP and for slabplanet runs - then we can add these cases to the slabplanet driver and still keep the AMIP driver clean

@juliasloan25
Copy link
Member Author

Disabling diagnostics feels like a workaround: the root cause of the problem is that we are adding a land model when there's no land. Is there any easy ways to fix that?

We can add two more branches to the big if/else here for the slabplanet_aqua and slabplanet_terra cases, but I didn't like this solution because it adds a lot of code to the driver for cases that aren't high priority. I think the solution is to create separate drivers for AMIP and for slabplanet runs - then we can add these cases to the slabplanet driver and still keep the AMIP driver clean

I think we could also come up with a different component model initialization approach - maybe we have smaller if/else chunks where we check the mode names and come up with flags to pass to generic component model constructors that dispatch off of them. E.g. we could have land_init(:bucket), land_init(nothing) that return different land model types

@juliasloan25 juliasloan25 force-pushed the js/longrun-fix branch 3 times, most recently from c98893e to 727070e Compare October 8, 2024 19:47
@trontrytel
Copy link
Member

Would it be possible to merge this as a temporary fix?

@juliasloan25
Copy link
Member Author

@Sbozzolo are you okay with merging this as a temporary fix? It would be nice to be able to run the longruns this weekend. I opened an issue about setting up the component models in a more reasonable way #1011 that we can address when we have the time

@Sbozzolo
Copy link
Member

Okay

@juliasloan25 juliasloan25 merged commit 48205a9 into main Oct 10, 2024
5 of 6 checks passed
@juliasloan25 juliasloan25 deleted the js/longrun-fix branch October 10, 2024 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants