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

Bug fixes for ISPOL and NICE test options. BFB physics #89

Merged
merged 4 commits into from
Jan 15, 2018

Conversation

njeffery
Copy link
Contributor

@njeffery njeffery commented Jan 8, 2018

  1. Adds missing parameters to init and write_parameters and init_zbgc
  2. Corrects seg fault when using iron tracers
  3. Fix to interpolation of 6 hourly data
  4. Typo in forcing flag for NICE and parameter name
  5. Adds nutrient data option for NICE
  6. Corrections test configuration for NICE and bgc
  7. Flag change to allow iron chemistry without DOC
  8. Fixes for SKL bgc: corrects growth max & adds flag warning

Are the code changes bit for bit, different at roundoff level, or more substantial?
Is the documentation being updated with this PR? (Y/N)
If not, does the documentation need to be updated separately? (Y/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:

1. Adds missing parameters to init and write_parameters and init_zbgc
2. Corrects seg fault when using iron tracers
3. Fix to interpolation of 6 hourly data
4. Typo in forcing flag for NICE and parameter name
5. Adds nutrient data option for NICE
6. Corrections test configuration for NICE and bgc
7. Flag change to allow iron chemistry without DOC
8. Fixes for SKL bgc: corrects growth max & adds flag warning
@njeffery
Copy link
Contributor Author

njeffery commented Jan 8, 2018

@eclare108213 : Hopefully the conflicts should be minor. I branched from master late last week.

@eclare108213
Copy link
Contributor

@njeffery The conflicts are coming from the refactoring of how constants are handled: #83
You should be able to pull the new changes from master into your fork, and you can look at the differences for configuration/driver/icedrv_forcing.F90 under the 'Files changed' tab at that pull request, to see where the new constants that you need are located or how to access them. If you have questions about this, please ask @apcraig or me. Thx

@eclare108213
Copy link
Contributor

eclare108213 commented Jan 8, 2018

Here are the lines that are conflicting, in case the 'resolve conflicts' button isn't working for you.
<<<<<<< njeffery_NICE
use icedrv_constants, only: nu_diag, nu_forcing, secday
use icedrv_constants, only: c0, c1, c2, c10, c100, p5, c4
use icedrv_constants, only: secday, Tffresh, qqqice, TTTice, rhos
=======
use icedrv_constants, only: nu_diag, nu_forcing
use icedrv_constants, only: c0, c1, c2, c10, c100, p5
>>>>>>> master

@apcraig
Copy link
Contributor

apcraig commented Jan 8, 2018

I believe the resolution should be to use the "master" version, but maybe add c4 to the list. Then, if there are any other new icepack constants that are needed, they should be set via a query to icepack_query_constants. Nicole, if you need me to look at this, let me know.

@njeffery
Copy link
Contributor Author

njeffery commented Jan 8, 2018 via email

@apcraig
Copy link
Contributor

apcraig commented Jan 8, 2018

It still shows as being conflicted. Did you push your changes to njeffrey_NICE branch on your fork?

@njeffery
Copy link
Contributor Author

njeffery commented Jan 8, 2018

After pulling, my branch still has atm_climatological which uses tffresh, qqqice, TTTice and rhos. Should I keep the line...
use icedrv_constants, only: tffresh, qqqice, TTTice, rhos ?

@apcraig
Copy link
Contributor

apcraig commented Jan 8, 2018

You should remove that line and give it a try. If you need any of those constants and they are not already set, then you will need to define local variables and use icepack_query_constants to set them. Again, happy to help if there are questions.

tffresh, qqqice, tttice and rhos are needed for option thermo1.
@njeffery
Copy link
Contributor Author

njeffery commented Jan 8, 2018

559a5ea resolves the conflicts.

@apcraig apcraig self-requested a review January 8, 2018 23:14
@apcraig
Copy link
Contributor

apcraig commented Jan 8, 2018

I still see conflicts, did you push the change to your branch? Looking at the files, icedrv_forcing still has this line,
use icedrv_constants, only: secday, Tffresh, qqqice, TTTice, rhos

@njeffery
Copy link
Contributor Author

njeffery commented Jan 9, 2018 via email

@apcraig
Copy link
Contributor

apcraig commented Jan 9, 2018

That line has been replaced by a query to the icepack constants. Please remove it and see if things still work.

@eclare108213
Copy link
Contributor

eclare108213 commented Jan 11, 2018

I tried this. The code compiles and runs and gets the same answers as in a baseline case from Nicole's branch. However there are still a couple of bugs, so I'll let @njeffery fix them before merging. Nicole, here's what to do, to get to where I was:

create baseline:
[eclare@pi-fe1 github]$ git clone -b njeffery_NICE https://github.com/njeffery/Icepack Icepack.NICE
[eclare@pi-fe1 github]$ cd Icepack.NICE
[eclare@pi-fe1 Icepack.NICE]$ icepack.create.case -ts base_suite -m pinto -testid n1 -bg nice.1
[eclare@pi-fe1 Icepack.NICE]$ cd base_suite.n1/
[eclare@pi-fe1 base_suite.n1]$ results.csh | grep FAIL
FAIL pinto_smoke_col_1x1_bgcISPOL_debug run
FAIL pinto_restart_col_1x1_bgcISPOL run-initial

[eclare@pi-fe1 github]$ git clone -b njeffery_NICE https://github.com/njeffery/Icepack Icepack.NICE2
[eclare@pi-fe1 github]$ cd Icepack.NICE2
[eclare@pi-fe1 Icepack.NICE2]$ git remote add upstream https://github.com/CICE-Consortium/Icepack
[eclare@pi-fe1 Icepack.NICE2]$ git pull upstream master
[eclare@pi-fe1 Icepack.NICE2]$ emacs -nw configuration/driver/icedrv_forcing.F90
fix conflict by deleting

    use icedrv_constants, only: secday, Tffresh, qqqice, TTTice, rhos

and adding c4 to the previous line
[eclare@pi-fe1 Icepack.NICE2]$ icepack.create.case -ts base_suite -m pinto -testid n2 -bc nice.1
[eclare@pi-fe1 Icepack.NICE2]$ cd base_suite.n2/
[eclare@pi-fe1 base_suite.n2]$ results.csh | grep FAIL
FAIL pinto_smoke_col_1x1_bgcISPOL_debug run
FAIL pinto_restart_col_1x1_bgcISPOL run-initial

@eclare108213
Copy link
Contributor

I will fix the conflict and execute this PR so that I can clean up the Icepack code, which undoubtedly would create more conflicts. The two failing tests noted above will still need to be fixed.

@eclare108213 eclare108213 merged commit 73e48b2 into CICE-Consortium:master Jan 15, 2018
@eclare108213 eclare108213 mentioned this pull request Jan 15, 2018
@eclare108213
Copy link
Contributor

@njeffery @chrisknewman
Please follow up on the two tests that are failing, issue #96, thanks.

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.

3 participants