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

Add a new diagnostic variable dvsdtd. #940

Merged
merged 9 commits into from
Mar 20, 2024
Merged

Conversation

dabail10
Copy link
Contributor

@dabail10 dabail10 commented Mar 2, 2024

For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers

PR checklist

  • Short (1 sentence) summary of your PR:
    This adds a new diagnostic variable dvsdtd so that the SIMIP variable sndmassdyn can be used.
  • Developer(s):
    dabail10 (D. Bailey)
  • 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.
    https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#1db0d9c13c0885c2da5c498a3fee0daa60788434
  • 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
    • 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 document the changes in detail, including why the changes are made. This will become part of the PR commit log.
    We finally added the SIMIP variable sndmassdyn. This required the computation of dvsdtd. Also, there were some unit issues with some of the sndmass variables.

Copy link
Contributor

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Is there not also a dvsdtt?
These changes will need to be made to all of the drivers, eventually.

@@ -419,6 +420,7 @@ subroutine alloc_flux
strintyU (nx_block,ny_block,max_blocks), & ! divergence of internal ice stress, y (N/m^2)
daidtd (nx_block,ny_block,max_blocks), & ! ice area tendency due to transport (1/s)
dvidtd (nx_block,ny_block,max_blocks), & ! ice volume tendency due to transport (m/s)
dvsdtd (nx_block,ny_block,max_blocks), & ! snow volume tendency due to transport (m/s)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I first looked at the PR, I thought this was salt, not snow. Do we need to distinguish the names of those now, in case we want to add a salt tendency term?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would not be hard to add a dvsdtt term. I was doing it specifically since there is the need for an SIMIP variable sndmassdyn. There is not a similar term for the thermodynamics. The snow melt and snow ice formation are called out in separate terms. I was using dvsdtd to be consistent with dvidtd.

Copy link
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In update state, I suggest

  • Add "if present" to dagedt calculation
  • With "if present", I think this syntax still sometimes creates run time problems with debug flags (it shouldn't)
   if (present(xxx)) xxx = yyy

would recommend

   if (present(xxx)) then
     xxx = yyy
   endif
  • Can we update the indentation in update_state? Seems it may be overdue.

@apcraig
Copy link
Contributor

apcraig commented Mar 12, 2024

Looks good. Is there not also a dvsdtt? These changes will need to be made to all of the drivers, eventually.

Since update_state is not backwards compatible, could you update all the call to update_state in all the other drivers. I would NOT add dvsdtd to those calls, but just update the arguments to use keywords= syntax. That should be adequate. Thanks.

@dabail10
Copy link
Contributor Author

Ah. Good point. I will do that. I will also work on dvsdtt at the same time.

for snow). Also, the calls to update_state have been updated in all
of the drivers. Note that I can only test the NUOPC/CMEPS and standalone
drivers, so it would be good if others could test.
@dabail10
Copy link
Contributor Author

I have added dvsdtt and fixed all of the calls to update_state in the drivers. The updated test results for the standalone driver are here:

https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#e68dd144aa1577111f5a87f93378afd15847b62e

apcraig and others added 2 commits March 14, 2024 16:12
@apcraig
Copy link
Contributor

apcraig commented Mar 18, 2024

I ran a test suite on Derecho and this all looks good. @dabail10, anything left to do? Any other comments? If not, I'll merge in the next day or two.

@dabail10
Copy link
Contributor Author

I believe this is everything here.

@apcraig apcraig merged commit 2209749 into CICE-Consortium:main Mar 20, 2024
2 checks passed
@dabail10 dabail10 deleted the dvsdtd branch March 27, 2024 14:36
DeniseWorthen pushed a commit to DeniseWorthen/CICE that referenced this pull request Apr 13, 2024
* Add dvsdtd diagnostic for SIMIP sndmassdyn

* Fix the units of SIMIP snow variables

* Fix calls to update_state

* Add present for dagedt and fix indents

* This PR has been updated to include dvsdtt (thermodynamic tendency
for snow). Also, the calls to update_state have been updated in all
of the drivers. Note that I can only test the NUOPC/CMEPS and standalone
drivers, so it would be good if others could test.

* Update the opticep unit test to be consistent with the latest changes.

---------

Co-authored-by: apcraig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants