-
Notifications
You must be signed in to change notification settings - Fork 135
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
Turn on snicar data table via USE_SNICARHC by default. Add cpl_frazil namelist. #881
Closed
Closed
Changes from all commits
Commits
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 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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.
What are the pros and cons of this change?
If users are not using dEdd and/or the snicar modifications, would they prefer that this be false? Would it make sense to leave it as false but document very clearly with snicar info that it is available?
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 downside to turning this on is that it takes longer to build. I just did a quick test. With the big table on, it takes 6 minutes to build CICE vs 2 minutes with it off. Just one sample on cheyenne with intel.
We talked about this a while ago and thought we wanted the full table to be on by default for users in case they set dEdd_snicar_ad. But we could have it off by default, but well documented. Also, if it's off and a user picks the "snicar" settings, it will get turned on automatically. That's why happens in testing, it's off by default, but the snicar tests all turn it on and have it compiled in.
Another option would be to have it on by default and document that users can save time compiling by turning it off if they don't need it.
Don't have a strong feeling either way nor a good sense of how often the snicar option will be used.
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.
Then I guess I don't understand why we need to change it. Is it always turned on automatically, or is that feature particular to the testing scripts? Regardless, it ought to be clearly documented, if it isn't already so.
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.
If a user sets up a case, then manually sets dEdd_snicar_ad in the namelist, then the run will abort at run time. It does provide a nice message, ' ERROR: USE_SNICARHC CPP required'. Again, I'm happy to have it off by default, but a while back, @eclare108213 requested that it be on by default. I've just been waiting for a chance to update it. :)
As currently implemented in this PR, it's ON by default. The scripts will turn if OFF if running a test or suite (not for a case). Then the snicar option turns it back ON. So, the behavior will be that a case will always have it on. Tests and suites will have it off except when it needs to be on for the snicar test (to speed up overall testing).
Prior to this implementation, default was OFF. It got turned on when running with -s snicar.
Users can also manually change ICE_SNICARHC in cice.settings to true/false to turn it ON/OFF.
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.
Maybe I'm going about this the wrong way. In Icepack, we need the "USE_SNICARNC" CPP to build the big table. Maybe what we need to do is have a CPP that turns off SNICARHC in Icepack? So the default in Icepack (without any CPPs) is that it's built. And a unique CPP turns that off. The point would be that users get the big table build unless they set a CPP to turn it off. Maybe that's what we need and how the default should be ON. Then we would keep the CICE behavior the same, off by default, on only if we need it. We would have to change the E3SM cpps though too. Thoughts?