Extract forecast reference time

[truth-quark]

First step to doing #100.

This PR is intended for facilitating testing functionality for fixing the forecast reference time.

As of late Sept 2024, work up to commit fd664c7 only provides a skeleton of unit tests. It doesn't meaningfully test any cube time changes. This PR includes some skipped tests as the testing required is unclear.

Work on this PR needs to answer & address the following:

• Is the time.units.calendar == 'gregorian' code branch & testing required?
• Is the time.units.calendar == 'proleptic_gregorian' code branch & testing required?
• What inputs should be tested?
• What outputs should be tested?
• What cube state/attrs should be tested?
• Is the code branch covered by test_fix_forecast_reference_time_standard() required?

These questions will help determine what work is required. Any comments/ideas welcome!

Fixing & testing this function is specified in #141.

[truth-quark]

Adding Martin as a reviewer to help clarify what is required.

[CodeGat]

This looks good to me. But note that I'm missing some of the domain knowledge needed to give a more 'conceptual' approval, so probably wait for others to give their review :D

[blimlim]

Pulling this section out into a separate function sounds like a good idea!

I'm still trying to understand the original version of the code, but have a couple of comments and questions so far:

It's a bit tricky to follow I think because of the dynamic overwrite of the PPfile.calendar method:

um2nc-standalone/umpost/um2netcdf.py

Lines 95 to 110 in d711729

	# Override the PP file calendar function to use Proleptic Gregorian rather than Gregorian.
	# This matters for control runs with model years < 1600.
	@property
	def pg_calendar(self):
	"""Return the calendar of the field."""
	# TODO #577 What calendar to return when ibtim.ic in [0, 3]
	calendar = cf_units.CALENDAR_PROLEPTIC_GREGORIAN
	if self.lbtim.ic == 2:
	calendar = cf_units.CALENDAR_360_DAY
	elif self.lbtim.ic == 4:
	calendar = cf_units.CALENDAR_365_DAY
	return calendar
	# TODO: Is dynamically overwriting PPField acceptable?
	PPField.calendar = pg_calendar

When I load in an ESM1.5 output file independently, e.g. /g/data/tm70/sw6175/nc_conversion/output_dirs_for_testing/output000/atmosphere/aiihca.paa1jan, I get cube.coord("time").units.calendar is standard. However, when I run um2nc on it, the if time.units.calendar == 'proleptic_gregorian' and refdate.year < 1600: branch is still executed because of the above overwrite.

Is the time.units.calendar == 'gregorian' code branch & testing required?

Because of the PPfile.calendar overwrite, I suspect we will never have a cube with a gregorian or standard calendar, and so I'm wondering if we can remove this branch entirely, or at least skip tests for it?

Is the time.units.calendar == 'proleptic_gregorian' code branch & testing required?

I think it would be good to test this branch eventually. Paleoclimate simulations usually use years before 1600, and so this branch would be used quite regularly.

---

@MartinDix, do you know why the forecast_reference_time coordinate is used to determine if the date is before 1600, rather than using the time coordinate directly?

um2nc-standalone/umpost/um2netcdf.py

Lines 367 to 369 in d711729

	reftime = cube.coord('forecast_reference_time')
	time = cube.coord('time')
	refdate = reftime.units.num2date(reftime.points[0])

I'm wondering if it's to ensure that the calendar of dump files isn't changed, e.g.

um2nc-standalone/umpost/um2netcdf.py

Lines 388 to 390 in d711729

	except iris.exceptions.CoordinateNotFoundError:
	# Dump files don't have forecast_reference_time
	pass

but thought it would be good to double check!

[truth-quark]

I'll link in some previous work on convert_propleptic() in issue #37.
There's part of a unit test here which might be useful...

[truth-quark]

Update: following the 9/10/2024 handover meeting, work on this will be split into 2 parts:

1. Extract the time/calendar functions & provide basic unit test scaffolding (this PR)
2. Retrofit testing in Retrofit unit tests for fix_forecast_reference_time() #141 & cover in a second PR.

[aidanheerdegen]

First pass, didn't have time to check the tests. Can take another look tomorrow.

[truth-quark]

First pass, didn't have time to check the tests. Can take another look tomorrow.

This PR has given us several open questions & highlighted that the requirements are not really known. This function partly blocks work to fix structural/architectural repairs too. To address all these, how is the following?

• Add code comments for all open questions
• Merge the comments & structural only changes via this PR
• Update Extract cubewrite() forecast reference time functionality #100 with details from this PR & leave it open
• Clarify requirements, what UM provides, what ACCESS NRI needs --> add to issue docs
• Use new PR for modifying the timing repair based on Extract cubewrite() forecast reference time functionality #100

How does that sound?

[blimlim]

I think the modularisation step looks good, and looks like it will behave exactly the same as before.

Setting up the tests and fixtures for this seems very tricky and intricate though, especially due to the pg_calendar overwrite. I'm wondering whether it would be easiest/fastest for this PR to just do the extraction into a separate function, and defer all of the tests to #141?

[truth-quark]

@blimlim ^ @aidanheerdegen do you want to check over the recent sets of changes & confirm if they cover the discussion threads here? I'll define "cover" as implements smaller fixes OR makes a note for a future fix.

[blimlim]

confirm if they cover the discussion threads here?

Changes look good

Would you be happy to add comments to the forecast_ref_time_coord and time_coord fixtures to note that their unit.calendar attributes will likely have to be changed in the future, since um2nc only ever uses the cf_units.CALENDAR_PROLEPTIC_GREGORIAN, cf_units.CALENDAR_360_DAY, and cf_units.CALENDAR_365_DAY calendars?

[truth-quark]

Would you be happy to add comments to the forecast_ref_time_coord and time_coord fixtures to note that their unit.calendar attributes will likely have to be changed in the future

That's gone in with 3820528.

[aidanheerdegen]

LFTM

[blimlim]

Looks good to me

[truth-quark]

Setting up the tests and fixtures for this seems very tricky and intricate though, especially due to the pg_calendar overwrite. I'm wondering whether it would be easiest/fastest for this PR to just do the extraction into a separate function, and defer all of the tests to #141?

Yep, initial attempts at testing were fiddly & nasty to set up the data structures. Approaching in small, logical steps, extracting helper funcs etc should make things better with gradual logic reduction.

I've also referenced #141 in the main body of this PR.