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

Intent inout in shortwave_dEdd_set_snow #227

Merged
merged 2 commits into from Oct 4, 2018
Merged

Intent inout in shortwave_dEdd_set_snow #227

merged 2 commits into from Oct 4, 2018

Conversation

dabail10
Copy link
Contributor

@dabail10 dabail10 commented Sep 27, 2018

This fixes a bug where the variables fs and hs should be intent inout in shortwave_dEdd_set_snow. This change is bfb in icepack except for hobart_nag_restart_col_1x1_alt03. This is with ktherm=1 and the topo poinds. All of the hobart_intel tests passed and were bfb. I'm not sure why this one case is not bfb.

  • Developer(s): D. Bailey

  • Please suggest code Pull Request reviewers in the column at right.

  • Are the code changes bit for bit, different at roundoff level, or more substantial? NOT BFB

  • To verify that this PR passes the initial QC tests, lease include the link to test results
    or paste in below the summary block from the bottom of the testing output.

  • Does this PR create or have dependencies on CICE or any other models?

  • Is the documentation being updated with this PR? (Y/N)
    If not, does the documentation need to be updated separately at a later time? (Y/N)

Note: "Documentation" includes information on the wiki and .rst files in doc/source/,
which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.

  • Other Relevant Details:

@eclare108213 REVISION TO THESE NOTES: As noted below, this merge is not BFB.

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.

A little worried about not bit-for-bit, but willing to approve. Did you do any bit-for-bit testing in CICE?

@dabail10
Copy link
Contributor Author

dabail10 commented Oct 4, 2018

Running the CICE base_suite again.

Dave

@dabail10
Copy link
Contributor Author

dabail10 commented Oct 4, 2018

So, I just compared back to some CICE baselines on cheyenne awhile ago. All of my tests are different with CICE master. So, maybe this is a compiler change on cheyenne? I guess I will go back and regenerate the baselines from CICE master again.

@dabail10
Copy link
Contributor Author

dabail10 commented Oct 4, 2018

Ok. So, 27 out of 32 compares failed in the CICE base_suite with this single change. The issue is the following in pseudo-code:

fs = c0
hs = c0

call set_snow(fs,hs)

...

later use fs/hs in a calculation

subroutine set_snow(fs,hs)

intent (in) fs,hs

hs = vsno / aice

if (hs >= hs_min) then
fs = c1
if (hs0 > puny) fs = min(hs/hs0,c1)
endif

So, when fs and hs are intent(in), this means the initialization earlier is ignored. In the subroutine "set_snow" hs is alway set, but fs might not be, i.e. when hs < hs_min. So, imagine a situation where the first time through hs = hs_min + eps. Then fs is set according to the min statement. However, the next time through maybe hs = hs_min - eps. Then fs is not reset to anything and it remembers it from the last call.

However, when fs and hs are intent(inout), they are always reset to zero before the call to "set_snow". I believe this explains the answer changes.

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.

OK, I assume it's right now and was incorrect before. It sounds like it changes answers. Thanks for running a test suite to check again.

@apcraig apcraig merged commit 0b4ee4e into CICE-Consortium:master Oct 4, 2018
lettie-roach pushed a commit to lettie-roach/Icepack that referenced this pull request Oct 18, 2022
* testing bib file

* fixing how to add new entries

* fixing names

* adjusting template
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.

None yet

3 participants