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

fix more icepack debug errors #132

Merged
merged 3 commits into from
Feb 8, 2018

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Feb 6, 2018

fix more icepack debug errors. in particular,
fix exp underflows and errors in icepack_algae (issue #126 ), icepack_brine (issue #129).
remove cdn_atm from atm_boundary_const (#125)
rename icedrv_restart_column to icedrv_restart_bgc (#100)
fix zqsn intent in subroutine init_vertical_profile (#103)
Developer(s): tcraig
Are the code changes bit for bit, different at roundoff level, or more substantial? bfb except bgcISPOL
Is the documentation being updated with this PR? (Y/N) N
If not, does the documentation need to be updated separately? (Y/N) N
"Documentation" includes information on the wiki and .rst files in doc/source/,
which are used to create the online technical docs at https://cice-consortium.github.io/Icepack/
Please suggest code reviewers in the column at right.
Other Relevant Details:

All tests pass on gordon, https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks, hash# a971864. All results are bit-for-bit except bgcISPOL.

bgcISPOL are bit-for-bit in the log files for the full-ITD case, land, and slab gridpoints.

bgcISPOL results are different for the icefree case, diverging first here,

< avg brine thickness (m)=        0.09317749194835656
---
> avg brine thickness (m)=        0.09317747986181293

@apcraig apcraig self-assigned this Feb 6, 2018
@apcraig
Copy link
Contributor Author

apcraig commented Feb 6, 2018

The non bfb in icefree case seems to be the change in brine implementation. In cases where values were underflowing to zero, they are now being set to e-10. We should make sure that value is correct. Maybe we should change this so exp(arg)-->0.

I will rebase my branch to the current master (my base is a few versions old), modify the brine implementation to set exps(arg) to zero in the computation in the brine when underflowing, and then retest.

@apcraig apcraig mentioned this pull request Feb 6, 2018
@eclare108213
Copy link
Contributor

This is good, thanks Tony. I think that changes that are nonBFB should be brought in one at a time, in their own PR. Optimally, they would also be tested individually in CICE before being pulled into Icepack. That's why I've been waiting to fix bugs until we ported the new Icepack interfaces. We might want to go ahead with this one anyhow, to save time, but in general I'd like to see the full testing procedure.

@apcraig
Copy link
Contributor Author

apcraig commented Feb 7, 2018

I refactored the icepack_brine exp underflow check and it's better. bgcISPOL now passes bit for bit for smoke runs but not restart runs (which are longer). I think that indicates it's a small difference, but I am happy to back it out so this PR is bit-for-bit. See https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks. a039157 is the latest implementation, a971864 is the prior implementation. comp failures are down but not eliminated.

@apcraig
Copy link
Contributor Author

apcraig commented Feb 7, 2018

@eclare108213, should I back out the non bfb changes (which will continue to produce underflows with gnu)? Do we want to test in CICE first? Do we want to go with this implementation? Do we want to try something else? I am happy to go with whatever you prefer.

I am testing the update of Icepack in CICE now. The tricky part too is that even if icepack is bit-for-bit, that change in Icepack applied in a CICE case might not be. We have to decide, moving forward, when we should be testing Icepack changes in CICE. Maybe it should be anytime we suspect a non bit-for-bit change, even if Icepack is bit-for-bit?

@eclare108213
Copy link
Contributor

@apcraig Your last question is one that we will probably answer by trial and error over time. Eventually we might figure out what types of changes are likely to be nonBFB in CICE even if they are BFB in Icepack. Are you already seeing an example of that? If so, then I think we need to test any change that we suspect might be nonBFB in CICE too, as part of the Icepack testing process. E.g. all the bugs that I didn't want to fix until now. I do think that in general we need to merge each suspicious one individually, so that we can more easily identify and back them out if problems come up later. For now, I recommend that you put in as many of the BFB-in-Icepack changes as you can, but hold off on the current nonBFB ones, do them one at a time. I hope it's not all of them but it might be. Can I help?

@apcraig
Copy link
Contributor Author

apcraig commented Feb 8, 2018

OK, this branch is now bit-for-bit in all tests. I have commented out the brine underflow so 1 gnu test fails, but all cases are bit-for-bit with 90ac834. See e05b6fb at https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks.

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.

2 participants