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

Precision #446

Merged
merged 10 commits into from
May 20, 2020
Merged

Precision #446

merged 10 commits into from
May 20, 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:
    Add history_precision namelist option.
  • 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.
    Test-Results.wiki/cice_dev/c38a514361.cheyenne.intel.20-05-14.154337.0.md
  • 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 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:
    The history file answers will only change when turning on double-precision history output, that is history_precision = 8.

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.

How were the pio changes tested in pio? Was binary mode tested?

Could we change set_nml.precision to set_nml.precision8 and then update the base_suite.ts testlist

Could the changes in cice.build, Macros.cheyenne_intel, and env.cheyenne_intel be reverted. I don't think they are needed for this PR and they will conflict with changes being made to implement standalone pio testing in a separate PR.

@dabail10
Copy link
Contributor Author

I was not able to get the pio interfaces working. I am just putting in the precision changes so we have them. It works with binary and netcdf. I can back out the Macros and env changes, but this is what I did to try to get everything to compile. Is this not the way we want to go?

@apcraig
Copy link
Contributor

apcraig commented May 17, 2020

The Macros and env changes I've made are similar, but not quite the same. Nothing wrong with what you have, but my mods are slightly different. It would be easier for me if you removed your changes.

@dabail10
Copy link
Contributor Author

Ok. Changes made.

@apcraig
Copy link
Contributor

apcraig commented May 17, 2020

There is also a change in cice.build that could be reverted if you have a chance. thanks.

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.

One other thing, the error check at line 516 in ice_init should be moved down in the subroutine and then it should set an abort flag. The idea is that we want all the namelist errors to be written out after parsing then for the model to abort. If it aborts one error at a time, it can take several runs to work thru the errors.

@dabail10
Copy link
Contributor Author

Is there any systematic way that the abort_flag number is set? Why does it jump from 21 to 30?

@apcraig
Copy link
Contributor

apcraig commented May 18, 2020

The abort_flag number is not important as long as it's not zero. It's better if it's unique. In fact, as presently implemented it will be the last error encountered if you have multiple. It would be better for this to be a character string that somehow accumulates the error list, but I don't think that's important to do at this point. The main point is that the errors are all written then later the code aborts if 1 or more is found. The abort_flag could be a logical too. My making it a number, I thought it would provide more info, and it does, but not as much as it might. Thanks!

@dabail10
Copy link
Contributor Author

One more dumb question. Why are there two abort_flag settings done after the print statements?

@apcraig
Copy link
Contributor

apcraig commented May 18, 2020

Not sure, I suspect it's historical. Some checks may have been originally done before the namelist is written to the log files and others after. That order probably was not changed when the abort_flag was implemented. We could probably clean that up a bit, but I'm not sure it's important. Actually, the kevp_kernel flag (21) probably should remain after the output because we want to see the original setting in the output before it's potentially changed. abort_flag=20 could be moved up or they all could be moved down.

@apcraig
Copy link
Contributor

apcraig commented May 18, 2020

I think this is ready to merge. @dabail10 any outstanding issues? @eclare108213 do you want to have a review? I am a little eager to merge this so I can pull it into my pio standalone testing and address any conflicts or other issues. If we want to wait a bit to merge this, I can wait (or I could merge from this dev branch to do some initial testing).

@apcraig apcraig merged commit 9ead578 into CICE-Consortium:master May 20, 2020
@apcraig apcraig mentioned this pull request Jun 7, 2020
@dabail10 dabail10 deleted the precision branch March 24, 2021 16:45
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