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 handling of finidat with cold starts #2870

Merged
merged 6 commits into from
Nov 12, 2024

Conversation

samsrabin
Copy link
Collaborator

Description of changes

This PR changes things so that:

  1. Cold-start FATES runs with finidat specified will not require -ignore_warnings, and the supplied finidat will actually be obeyed.
  2. Cold-start non-FATES runs will not be allowed, even with -ignore_warnings.

Specific notes

Contributors other than yourself, if any: @mvdebolskiy

CTSM Issues Fixed:

Are answers expected to change (and if so in what way)? Yes: In cold-start FATES runs where the user was supplying finidat, that setting will actually be obeyed.

Any User Interface Changes (namelist or namelist defaults changes)? No

Does this create a need to change or add documentation? Did you do so? No

Testing performed, if any: Added checks to build-namelist_test.pl, which now pass.

@samsrabin samsrabin requested a review from ekluzek November 8, 2024 20:27
@samsrabin samsrabin marked this pull request as draft November 8, 2024 20:29
@samsrabin
Copy link
Collaborator Author

@ekluzek I've converted this to a draft while I figure out what the deal is with those extra commits up top.

Currently fails. We want FATES to work and BGC to fail, even when ignoring warnings.
Resolves ESCOMP#2856, where such runs were getting their finidat overwritten with ' '.
Previously, this only produced a warning, which could be overridden.
@samsrabin samsrabin force-pushed the fix-fates-cold-finidat branch from aa79626 to 33fd95d Compare November 8, 2024 20:33
@samsrabin samsrabin marked this pull request as ready for review November 8, 2024 20:33
@samsrabin
Copy link
Collaborator Author

Okay, fixed!

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.

@samsrabin this is great thanks for working on this. Sorry you had to work in perl, but I appreciate your working on it anyway. I have liked perl in the past and I'm still probably better in perl than in python, but I also know that ditching perl is going to be best for CTSM. But, we have to manage the current code too...

I also argue with myself below about the tests, but rested on let's bring this in as is.

Should this come to master though or b4b-dev?

bld/unit_testers/build-namelist_test.pl Show resolved Hide resolved
@samsrabin
Copy link
Collaborator Author

Should this come to master though or b4b-dev?

@ekluzek I lean toward master, because although it doesn't change answers in our tests, it will change answers for anyone like Matvey who had tried to do cold-start FATES runs with finidat by setting -ignore_warnings. This is because their finidat setting will now actually be respected, rather than being overwritten to ' '.

@ekluzek ekluzek added enhancement new capability or improved behavior of existing capability priority: Immediate Highest priority, something that was unexpected science Enhancement to or bug impacting science usability Improve or clarify user-facing options size: small labels Nov 8, 2024
@ekluzek ekluzek added this to the cesm3_0_beta05 milestone Nov 8, 2024
@ekluzek
Copy link
Collaborator

ekluzek commented Nov 8, 2024

Ahhh, good point master would give it more visibility. And since this makes a standard thing impossible for FATES it's good to give it more visibility and that it's fixed.

You will need to make a full ChangeLog and tag for it. But, I see your point where this warrants that even though it's small. I added it to upcoming tags and you might as well do this after b4b-dev comes in. This also brings it in sooner than the 2 week timeline for b4b-dev right now.

@samsrabin samsrabin merged commit 931b425 into ESCOMP:master Nov 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new capability or improved behavior of existing capability priority: Immediate Highest priority, something that was unexpected science Enhancement to or bug impacting science size: small usability Improve or clarify user-facing options
Projects
Status: Done (non release/external)
Development

Successfully merging this pull request may close these issues.

FATES will never start from specified finidat
2 participants