-
Notifications
You must be signed in to change notification settings - Fork 345
Add default handling for fates firemodes in lightning resolution #3606
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
Open
mvdebolskiy
wants to merge
5
commits into
ESCOMP:b4b-dev
Choose a base branch
from
mvdebolskiy:fix-nml-light-res-fates
base: b4b-dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
04d6941
add default handling for fates firemodes in lightning resolution
mvdebolskiy c3cb928
add checks for the vars not being already defined.
mvdebolskiy 1633a0e
remove extra confitions.
mvdebolskiy 6497c67
Merge branch 'b4b-dev' into fix-nml-light-res-fates
ekluzek e4218c6
Merge branch 'b4b-dev' into fix-nml-light-res-fates
mvdebolskiy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense for this to go here. But, I also think that the call to setup_logic_fates could be moved in front of this as an alternative way to do it. Since, the lightning resolution for fire is now used for both BGC and FATES I think we should add more comments to the header about this being the case. This was originally designed as only being for BGC and not FATES. So it would be good to clarify that in the header and some of the comments in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking the order could be changed, but fates_spitfire_mode isn't set until much later. So I don't see a way to merely change the order of the calls to make this work. You might have already tested that as the easiest solution.
In any case since both FATES and BGC need to determine the lightning resolution, they both need to be taken into account here so this change makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other thing I realized is that to protect against reordering going wrong -- we can add some assert statements to the subroutines to ensure the needed things are appropriately set. It would be simple to add a few statements to start doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So @mvdebolskiy there's a couple small things that I can see doing that would be good to bring in with this. I suggest the way forward, would be for me to push to your branch -- but have you look over my changes at the end and make sure you approve. I could also give you some instructions and have you implement it -- and then go back and forth to get it right. But, I think it would be easier to have me do those couple things here. So how does that sound for process?