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

Moved calculation of sicen and trcrn_bgc to the loop where they are used #507

Merged
merged 1 commit into from
Aug 26, 2020

Conversation

TillRasmussen
Copy link
Contributor

Moved calculation of sicen and trcrn_bgc to the loop where they are used. The current construction did not use the calculated values as they were defined private and overwritten at each i/j

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:
    Resolve warning
  • Developer(s):
    tar
  • Suggest PR reviewers from list in the column to the right.
    @apcraig @dabail10
  • Please copy the PR test results link or provide a summary of testing completed below.
    This is a bug fix of the init_bgc. I have not been running a test with bgc but the code compiles on gfortran, intel and cray. It will change the initialization of bgc.
    I hope that @dabail10 will review that the code work as intended. I have one concern this is that the initial calculation was imbedded in a "if (.not. restart_bgc)" clause. This is not the case anymore, however it is only used here.
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • [x ] more substantial (anticipated)
  • Does this PR create or have dependencies on Icepack or any other models?
    • Yes
    • [ x] 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
    • [ x] No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

…sed. The current construction did not use the calculated values as they were defined private and overwritten at each i/j
Copy link
Contributor

@dabail10 dabail10 left a comment

Choose a reason for hiding this comment

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

This looks good. I missed this when I added sicen and trcrn_bgc to the OMP PRIVATE statement earlier.

@apcraig
Copy link
Contributor

apcraig commented Aug 19, 2020

Do we want to do a qc test with bgc? It looks like this could have significant impacts on the initial condition. Does the initial condition change when using a restart file? We should probably check exact restart as well.

@TillRasmussen
Copy link
Contributor Author

Do we want to do a qc test with bgc? It looks like this could have significant impacts on the initial condition. Does the initial condition change when using a restart file? We should probably check exact restart as well.

This will impact the initial condition as the value of sicen and trcrn_bgc were undefined in the previous version.
If a restart file is used it is read afterwards (line 953), thus I dont think that this will change the result.

@apcraig
Copy link
Contributor

apcraig commented Aug 24, 2020

I ran a full test suite on conrad with this change and all results including the bgc configurations are bit-for-bit. Looking at a bgcz test, the initial condition seems to be "runtype=initial, ice_ic=none". So, just for further clarification, is this what we expect?

@eclare108213
Copy link
Contributor

If the BGC is being started from a no-ice condition, then I wouldn't expect the answers to change. It sounds like the modifications here only affect initializations from a state with existing ice, which aren't restarting BGC (restarts should overwrite the modified initialization). I don't think (but we should check) that we have test cases for BGC starting from existing ice, mainly because of the headaches associated with getting the initial conditions correct - @njeffery and I have generally always started from no-ice or from restarts with the BGC-relevant info in them, to let things evolve naturally. @njeffery please correct me if I'm wrong about that...

@njeffery
Copy link
Contributor

There is an option to activate BGC from a spin-up run with no BGC. Set restart_bgc = false and scale_bgc = true. This option assumes that initial concentrations of nutrients in existing sea ice are proportional to the salinity profile and scaled by the ocean SSS divided by the respective ocean nutrient. I'd still disregard the first 10 years or so of the output.

@apcraig
Copy link
Contributor

apcraig commented Aug 25, 2020

In preparation for the next release, I will merge this tomorrow morning unless any other concerns are expressed before then. Thanks.

@eclare108213
Copy link
Contributor

It would be good to add a test for the case that @njeffery describes above, but that does not have to be done for this PR. Better to get the bug fixed now.

@apcraig apcraig merged commit cfca1a8 into CICE-Consortium:master Aug 26, 2020
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.

5 participants