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

Snow melt computation bug. #328

Merged
merged 17 commits into from
Jul 23, 2020
Merged

Snow melt computation bug. #328

merged 17 commits into from
Jul 23, 2020

Conversation

dabail10
Copy link
Contributor

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:
    When the sea ice completely melts away in the thermodynamics, the final little bit of snow melt is not added to the melts array.
  • 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/icepack_by_mach_forks#izumi
  • 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 CICE or any other models?
    • 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/.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:
    This changes answers in 18 of the base_suite tests in Icepack. I have run the QC suite within CICE and it passes. It's interesting that the biggest differences are in the Weddell Sea, but they are still on the order of 1cm. Here are the plots:

ice_thickness_izumi_nag_smoke_gx1_48x1_medium_qc qc_base

ice_thickness_izumi_nag_smoke_gx1_48x1_medium_qc qc_te

ice_thickness_izumi_nag_smoke_gx1_48x1_medium_qc qc_base_minus_izumi_nag_smoke_gx1_48x1_medium_qc qc_te

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.

A one-line bug fix - nice. Thanks for finding and fixing this. Who is J. Zhu?

@dabail10
Copy link
Contributor Author

Jiang Zhu is a new project scientist at NCAR. He found this while working on his PhD and bringing in water isotopes into CICE.

Copy link
Contributor

@proteanplanet proteanplanet left a comment

Choose a reason for hiding this comment

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

Nice catch

@apcraig
Copy link
Contributor

apcraig commented Jul 17, 2020

I just want to clarify the non bit-for-bit results. It looks like a40bbf9 was bit-for-bit except for 18 tests with the pgi compiler (intel, nag, and gnu were bit-for-bit). But then the regression tests with faa181f were all bit-for-bit against the exact same baseline version except for 1 test. I don't understand these results. Why are the test results not consistent and why are the pgi results not bit-for-bit when the other compilers are?

Also, I guess I would expect all answers to change. Was a test suite run with CICE to see whether the results change for all configurations?

I think we can merge this, we know it's a bug fix. I guess I'm just trying to better understand the test results we're seeing and to make sure it's consistent with what we expect.

@dabail10
Copy link
Contributor Author

I only ran the QC test for one configuration. I will run the CICE base_suite as well.

@dabail10
Copy link
Contributor Author

The CICE base_suite will have to wait until Monday when cheyenne is back up.

@dabail10
Copy link
Contributor Author

I had that wrong. I already did the QC test. I am running the base_suite on cheyenne against the baselines.

@dabail10
Copy link
Contributor Author

Here are the CICE base_suite results:

https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_mach_forks#cheyenne

Not sure why gnu only has two answer changing cases. It looks like it just takes longer for the answer changes to come up and hence show up in the 90 day and 1 year tests?

@apcraig
Copy link
Contributor

apcraig commented Jul 23, 2020

Thanks for doing the extra testing. Good to know what kinds of changes we should expect when we pull it into CICE.

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.

4 participants