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

Hycom forcing v2 + small adds / corrections #230

Merged
merged 5 commits into from
Nov 16, 2018

Conversation

mhrib
Copy link
Contributor

@mhrib mhrib commented Nov 6, 2018

This PR replace #224:

--o--

  1. Subroutines for reading HYCOM initial SST+SSS forcing and HYCOM atmospheric forcing input in NetCDF format
  2. Minor corrections in popgrid_nc in ice_grid.F90: All records are the first - unlike for binary file formats
  3. Add m_per_sec as precipitation unit option
  4. Add "h" (hour) as accepted unit for restart-file-dump interval
  5. Add optional input argument to "ice_open" subroutine in ice_read_write.f90. This makes it ready to read binary files structured in fixed size blocks as eg. used by HYCOM.

--o--

Developer(s): Mads Hvid Ribergaard, DMI

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

Tony??

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

Please include the link to test results or paste the summary block from the bottom of the testing output below.

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

Is the documentation being updated with this PR? (Y/N)

Yes. File updated: doc/source/user_guide/ug_case_settings.rst

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:

ad 1) HYCOM forcing:
Do not conflict with other stuff. But nice to have for DMI :-)

ad 2) "popgrid_nc" record-numbers in ice_grid.F90
Corrected all nrec to 1, as it is the first record that is needed for each variables: htn, hte, kmt, etc.
There is ONLY one record for each variable! This is different from "popgrid"-binary, which consists of 7 variables - ie. need a specific record number.

ad 3)+4): Simple adds

ad 5) Nice to have option, which DMI have used previous and may continue to use.

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.

This looks OK, but I may run a test suite on the changes. I assume there is no way to test the hycom forcing option at this point within the consortium? Is it worth adding some input data and setting that up?

@@ -362,12 +364,15 @@ Table of namelist options
"", "``monthly``", "monthly forcing data", ""
"", "``ncar``", "NCAR bulk forcing data", ""
"", "``oned``", "column forcing data", ""
"", "``hycom_atm``", "HYCOM atm forcing in NetCDF format", ""
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer this just be called "hycom" not "hycom_atm". This is how you did it for ocn_data_type and is consistent with the other options (e.g. "ncar" or "clim").

Copy link
Contributor

@duvivier duvivier left a comment

Choose a reason for hiding this comment

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

I left two specific comments for changes to "hycom_am" and requesting you fix a typo I just noticed but not introduced by you. thanks!

@mhrib
Copy link
Contributor Author

mhrib commented Nov 6, 2018 via email

@mhrib
Copy link
Contributor Author

mhrib commented Nov 6, 2018 via email

@apcraig apcraig merged commit ef15baf into CICE-Consortium:master Nov 16, 2018
@mhrib mhrib deleted the HYCOM_forcing_v2 branch April 16, 2020 17:33
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