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 reading of mixed layer forcing file. #578

Merged
merged 12 commits into from
Mar 24, 2021

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:
    This fixes a bug where the mixed-layer forcing was not initialized correctly as the array was not allocated.
  • 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.
    https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_mach_forks#cheyenne
  • 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:
    I have added a new test, set_nml.ml that turns on oceanmixed_ice with a SOM forcing file from a CESM run. The main change is to move the call to init_forcing_ocn in CICE_InitMod.F90. Also, the default has changed to use a 2D forcing file with everything at the gridcell centers. I've also added some helpful extra diagnostic information in the netCDF read routines.

@dabail10 dabail10 self-assigned this Mar 16, 2021
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.

Looks OK to me. Recommend additional reviews for model setup.

Copy link
Contributor

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

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

In ice_forcing.F90, the currents might still be too strong, but you can try it like this.

Instead of moving init_forcing_ocn later in the subroutine, would it work to move init_forcing_atmo up? sst is needed in init_state.

Doesn't ocn_data_dir need to be generic in set_nml.ml, as for other (atmo) forcing?

@apcraig apcraig linked an issue Mar 17, 2021 that may be closed by this pull request
@dabail10
Copy link
Contributor Author

Very good points. I'm actually now thinking I could move the call to allocate the arrays to init_forcing_ocn instead and leave the calls in the current location in CICE_InitMod.F90. You are right about ocn_data_dir as well.

@dabail10
Copy link
Contributor Author

Sorry for the repeated commits/pushes. I had troubles with conflicts from the new time manager stuff.

@dabail10
Copy link
Contributor Author

Looks like I still have some issues.

@eclare108213
Copy link
Contributor

@dabail10 since you've gotten some runs completed, have the issues mentioned above been fixed?

@dabail10
Copy link
Contributor Author

I think the code is fixed, but we don't have the forcing file on Zenodo yet.

@dabail10 dabail10 closed this Mar 24, 2021
@dabail10 dabail10 deleted the oceanmixed branch March 24, 2021 16:45
@apcraig
Copy link
Contributor

apcraig commented Mar 24, 2021

I'm confused, why was this closed and not merged?

@dabail10
Copy link
Contributor Author

Not sure. I think it needs the forcing file?

@apcraig
Copy link
Contributor

apcraig commented Mar 24, 2021

@dabail10, it looks like you closed this. we can reopen it. I thought this had to be merged regardless of whether there is an available file on zenodo.

@dabail10
Copy link
Contributor Author

I can reopen. Not sure what happened.

@dabail10 dabail10 restored the oceanmixed branch March 24, 2021 17:15
@dabail10
Copy link
Contributor Author

I see what happened. I thought it was merged and I deleted the branch. My bad.

@dabail10 dabail10 reopened this Mar 24, 2021
ocn_data_type = 'ncar'
ocn_data_format = 'nc'
ocn_data_dir = 'ICE_MACHINE_INPUTDATA/CICE_data/forcing/gx1'
oceanmixed_file = 'pop_frc.gx1.nc'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need have a standard name for the oceanmixed_file and then commit to Zenodo.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about ocean_forcing_gx1.nc?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be named similarly to the atmospheric forcing, but that is not standardized. It could follow the JRA tx1 example,
CESM_ocean_monthly_forcing_gx1.nc -- probably overkill since its location in the forcing directory structure would identify it: CICE_data/forcing/gx1/CESM/MONTHLY/
So ocean_forcing_gx1.nc works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eclare108213 mentioned having 2D in the name as well. Then if we put it under the forcing/gx1 area, we shouldn't need the "gx1" label.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want a filename that doesn't rely on it's directory location to describe it. We also don't want the same filename in multiple directories. So we want to avoid having multiple "ocean_forcing.nc" files in multiple directories. When the file is separated from it's directory, it should be unique (relative to other input data files) and still somewhat descriptive. I am completely fine with something like "CESM_ocean_monthly_climatology_forcing_gx1_210324.nc". But it need not be that descriptive. It's always a balance. When/if we update this file in the future, we'll need to give it a new name as well. We need to make sure we never have multiple versions of the same filename, ever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the Kay et al. 2015 reference to the CESM-LE in the metadata. I think the metadata can be very descriptive as well as the Zenodo entry. I believe we should keep the filename simple.

@eclare108213
Copy link
Contributor

eclare108213 commented Mar 24, 2021 via email

@eclare108213
Copy link
Contributor

eclare108213 commented Mar 24, 2021 via email

@dabail10
Copy link
Contributor Author

I like that. I will fix the PR.

@dabail10
Copy link
Contributor Author

Updated set_nml.ml option. The forcing file has been added to Zenodo at 10.5281/zenodo.4633898.

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.

It looks like this is ready to merge. @eclare108213 ?

@eclare108213
Copy link
Contributor

The new forcing file and location needs to be added to the input data wiki page. Otherwise this is ready. Thank you!

@eclare108213 eclare108213 merged commit 1ae2f03 into CICE-Consortium:master Mar 24, 2021
@dabail10
Copy link
Contributor Author

The forcing file information was added to the input data page.

@dabail10 dabail10 deleted the oceanmixed branch March 25, 2021 15:15
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.

init_forcing_ocn needs to be called after init_forcing_atm
3 participants