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

Change calendar attr from THIRTY_DAY_MONTHS to 360_DAY #574

Merged
merged 1 commit into from
Sep 16, 2020

Conversation

adcroft
Copy link
Member

@adcroft adcroft commented Sep 8, 2020

  • The calendar attribute in netcdf files is not CF compliant when set to THIRTY_DAY_MONTHS. The attribute value should be 360_DAY.

Description
Changed the result of valid_calendar_types() from 'THIRTY_DAY_MONTHS' to '360_DAY' which has the desired effect of correcting the netcdf attribute without any other input or code changes. The new value is CF compliant: https://cfconventions.org/Data/cf-conventions/cf-conventions-1.8/cf-conventions.html#calendar

Fixes #573

How Has This Been Tested?
Passes Travis-CI tests and submitted branch resolves the NOAA-GFDL/MOM6#1203 when MOM6 is run in NeverWorld2 configuration.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • make distcheck passes

- The calendar attribute in netcdf files is not CF compliant when
  set to THIRTY_DAY_MONTHS. The attribute value should be 360_DAY.
Copy link
Contributor

@bensonr bensonr left a comment

Choose a reason for hiding this comment

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

While I am okay with the change, I need to hear from the workflow team whether PP will be affected by this change in the calendar type attribute before approving.

@bensonr
Copy link
Contributor

bensonr commented Sep 8, 2020

@ceblanton do you have a comment on how PP might be affected if we change a calendar type attribute from THIRTY_DAY_MONTHS to 360_DAY?

@ceblanton
Copy link

Hi @bensenr, thanks for the check. FRE post-processing should not be affected by this change. frepp gets its calendar type specification from the coupler namelist. It would probably be better to get it from the NetCDF file itself, but is lucky for this change!

Copy link
Contributor

@bensonr bensonr left a comment

Choose a reason for hiding this comment

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

With the comment from @ceblanton, I will approve this PR.

@thomas-robinson
Copy link
Member

@adcroft please check all of the boxes, even if they aren't applicable to your PR.

What version of the code did you fork to make this change? Was it 2019.01.03?

@thomas-robinson
Copy link
Member

@adcroft if you are using 2019.01.03 (or 2019.01.02) for your model runs, we can patch that and make 2019.01.04. If you want to just put this on the main development branch, we are about to release 2020.03 (assuming there are no bugs found) on Thursday or Friday. Your update then would either be a part of 2020.04, and/or a patch 2020.03.01. If we patch 2019.01.03, then we will put this in 2020.04. If this is unclear, then you can send me a chat message to discuss it in person and clear up and confusion.

@adcroft
Copy link
Member Author

adcroft commented Sep 10, 2020

@thomas-robinson I made this on top of master. Our plan is to catch MOM6 up to 2020 once NCAR has the change for 2019.01.03. Since we have a work around for now I suggest we minimize divergence and leave this in the normal development pipe to appear whenever you next tag.

@thomas-robinson thomas-robinson added this to the 2020.03 milestone Sep 10, 2020
@thomas-robinson
Copy link
Member

Thanks for the clarification @adcroft . I'll merge it in for our next round of testing.

@thomas-robinson thomas-robinson merged commit 521a151 into NOAA-GFDL:master Sep 16, 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.

THIRTY_DAY_MONTHS calendar is not CF compliant
5 participants