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

Turn on snicar data table via USE_SNICARHC by default. Add cpl_frazil namelist. #881

Closed
wants to merge 1 commit into from

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Oct 9, 2023

PR checklist

  • Short (1 sentence) summary of your PR:
    Turn on snicar data table via USE_SNICARHC. Add cpl_frazil namelist
  • Developer(s):
    apcraig
  • Suggest PR reviewers from list in the column to the right.
  • Please copy the PR test results link or provide a summary of testing completed below.
    All results bit-for-bit, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#deb247bcec381615d01006bfa15dddf3a4f068fd
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on Icepack or any other models?
    • Yes, but already included in Icepack
    • No
  • Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes
    • No
  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

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

…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
@apcraig
Copy link
Contributor Author

apcraig commented Oct 9, 2023

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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

@apcraig apcraig marked this pull request as draft October 12, 2023 22:10
@apcraig
Copy link
Contributor Author

apcraig commented Oct 13, 2023

Closing, both the USE_SNICARHC and update_ocn_f will be updated via separate PRs. See #885 and CICE-Consortium/Icepack#390.

@apcraig apcraig closed this Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants