-
Notifications
You must be signed in to change notification settings - Fork 132
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
Conversation
…ult. This is turned off by default for tests or test suites and turned on only if needed to reduce compile time during testing. Update the frazil term in CICE diagnostics and history. This may need a further revision. Remove use of update_ocn_f in the icepack_step_therm2 call in CICE, now set via Icepack parameters. Add cpl_frazil namelist to CICE, passed into Icepack to set coupling type. Add full snicar and e3sm tests
This provides an initial fix to the update_ocn_f diagnostics and history updates. Other changes may be coming later, see CICE-Consortium/Icepack#390 |
@@ -44,7 +44,7 @@ setenv ICE_COMMDIR mpi | |||
if (${ICE_NTASKS} == 1) setenv ICE_COMMDIR serial | |||
|
|||
### Specialty code | |||
setenv ICE_SNICARHC false # compile with big hardcoded snicar table | |||
setenv ICE_SNICARHC true # compile with big hardcoded snicar table |
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.
if it's off and a user picks the "snicar" settings, it will get turned on automatically.
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?
Closing, both the USE_SNICARHC and update_ocn_f will be updated via separate PRs. See #885 and CICE-Consortium/Icepack#390. |
PR checklist
Turn on snicar data table via USE_SNICARHC. Add cpl_frazil namelist
apcraig
All results bit-for-bit, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#deb247bcec381615d01006bfa15dddf3a4f068fd
Turn on the big snicar data table via USE_SNICARHC in Icepack by default. This is turned off by default for tests or test suites and turned on only if needed to reduce compile time during testing.
Update the frazil term in CICE diagnostics and history. This may need a further revision.
Remove use of update_ocn_f in the icepack_step_therm2 call in CICE, now set via Icepack parameters.
Add cpl_frazil namelist to CICE, passed into Icepack to set coupling type.
Add snicar and e3sm tests