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 and improve anomaly forcings for ISSP cases #292

Merged
merged 24 commits into from
Mar 19, 2025

Conversation

samsrabin
Copy link
Contributor

@samsrabin samsrabin commented Aug 9, 2024

Description of changes

Fixes a bug, and also makes it much simpler to run land-only SSP cases.

Specific notes

Contributors other than yourself, if any: @ekluzek

CDEPS Issues Fixed (include github issue #):

Are there dependencies on other component PRs (if so list): This doesn't depend on other components. However, I will soon submit a PR to CTSM that depends on this branch. (See issue ESCOMP/CTSM#2301.)

Are changes expected to change answers (bfb, different to roundoff, more substantial): Substantial, but only if people were being bitten by #258 before.

Any User Interface Changes (namelist or namelist defaults changes): Yes.

Adds anomaly_forcing options:

  • Anomaly.Forcing.cmip5.rcp45
  • Anomaly.Forcing.cmip6.ssp126
  • Anomaly.Forcing.cmip6.ssp245
  • Anomaly.Forcing.cmip6.ssp370
  • Anomaly.Forcing.cmip6.ssp585

Removes all variable-specific anomaly_forcing options.

Testing performed (e.g. aux_cdeps, CESM prealpha, etc):

  • New CTSM testmods using these work correctly. Existing testmod, datm_ssp126_anom_forc, is bit-for-bit identical to previous version (ctsm5.2.015).
  • On top of existing aux_cdeps test of SSP585 compset, added the 3 other SSP compsets. All aux_cdeps SSP tests show diffs as expected from cdeps1.0.34.

Hashes used for testing:

  • Various throughout process.

Remaining work

  • Make it so that anomaly_forcing is automatically set for each ISSP compset
  • If we care that it's not putting anomaly_forcing in the generated datm_in: Fix that. (Note that the anomaly forcing file IS correctly being put in datm.streams.xml.)

@samsrabin
Copy link
Contributor Author

samsrabin commented Aug 9, 2024

@ekluzek, a few questions:

  • What sort of changes/additions would you like me to make to aux_cdeps?
  • Does it make sense to keep the Anomaly.Forcing.Precip, Anomaly.Forcing.Temperature, etc. entries in stream_definition_datm.xml? If so, I'll need to rename them so they're specific to rcp45, then add equivalent ones for each SSP. Seems messy!
  • To make the ISSP compsets automatically use the right anomaly_forcing, would I do something like this in namelist_definition_datm.xml under <entry id="anomaly_forcing">?
<default_value>''</default_value>
<value compset="^SSP126_">Anomaly.Forcing.cmip6.ssp126</value>

@jedwards4b
Copy link
Contributor

@samsrabin is this ready- if not can you make it a draft please.

@samsrabin samsrabin marked this pull request as draft August 22, 2024 14:51
Copy link
Contributor

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

It's not clear to me that anomaly_forcing will always be defined and be an array.

@samsrabin
Copy link
Contributor Author

samsrabin commented Sep 5, 2024

Force-pushed to remove premature merge of main into this branch, which was interfering with my testing.

@samsrabin samsrabin marked this pull request as ready for review September 6, 2024 17:21
@samsrabin samsrabin requested a review from jedwards4b September 6, 2024 17:21
@samsrabin
Copy link
Contributor Author

@ekluzek This is ready, if you'd like to review!

@samsrabin
Copy link
Contributor Author

Note that this is up-to-date with cdeps1.0.34. If you'd like me to merge in the latest tag, let me know and I'll redo the testing.

@samsrabin
Copy link
Contributor Author

@jedwards4b This is ready to review, and @ekluzek will also make some time to look at it in the next few days. It would be nice if we can get it in soon.

@ekluzek ekluzek added bug Something isn't working enhancement New feature or request CESM Only Responsibility: CTSM Responsibility to manage and accomplish this issue is the CTSM Software group labels Nov 4, 2024
Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

Really glad you took this on. This is so great to have coming in. I'm suggesting some changes.

I think it would be better to be data driven rather than code driven. And I layout a mostly fleshed out plan on how to do that. You'll need to make sure that all works, but I think it will and it's an improvement over some of the limitations of a part of what's in here.

We should get together to go over all of this. I also have some questions about one set of changes that would be good for you to explain.

@samsrabin
Copy link
Contributor Author

Re-requesting review from @ekluzek, because it seems like I've already addressed his concern. If approved, I'll continue by merging in the latest CTSM tag into ESCOMP/CTSM#2686 and the corresponding CDEPS tag here.

@samsrabin
Copy link
Contributor Author

Re-pinging @ekluzek for review, since @jedwards4b seems to think things are fine as is.

Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

Thanks for your work here @samsrabin. I really like the simplification you figured out for this. And I like that AF forcing streams have the SSP name in them. And I like that you added the "none" option.

There are two things that I think deserve a discussion, so I'll make them into issues, and if we think they are important could come in a future PR.

The only suggestion I have at this point is to add a comment about the new tests added. But, still marking this as approve so you don't have to wait on me.

cmip5 and cmip6 anomalies were generated only with GSWP3, so buildnml now errors if DATM_MODE isn't CLMGSWP3v1. However, the user can override this by setting the XML variable DATM_MODE_ANOMALY_FORCING_MISMATCH to TRUE.
Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

This is a nice addition of good error checking for Anomaly Forcing (AF), which I think is really important to have in place. So thanks for doing this! The code is clean and clear, so good work.

I put two questions in. Longer term I'd like to have the type of forcing for AF to be in the XML data in some form, so it isn't hardcoded in the python. But, as there's only one option now, it doesn't need to start that way.

The other question is the importance of the new namelist variable option. If this is really going to be rarely used it might be good to NOT have it in the XML. But, have clear instructions on how to do. This makes the model easier to use for people by leaving off the most complex options if we think they won't be used often. But, that's up to you.

So thanks for your work here. This is great to see coming in.

@samsrabin
Copy link
Contributor Author

@jedwards4b I think this is ready!

@jedwards4b
Copy link
Contributor

@samsrabin can you merge main into your branch or give me permission to push to your fork?

@samsrabin
Copy link
Contributor Author

@jedwards4b Done!

@jedwards4b jedwards4b merged commit cde4ac6 into ESCOMP:main Mar 19, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CESM Only enhancement New feature or request Responsibility: CTSM Responsibility to manage and accomplish this issue is the CTSM Software group
Projects
None yet
3 participants