Skip to content

Don't guess instantaneous time coordinate bounds#2111

Open
Scott Wales (ScottWales) wants to merge 4 commits into
MetOffice:mainfrom
ScottWales:time-bounds
Open

Don't guess instantaneous time coordinate bounds#2111
Scott Wales (ScottWales) wants to merge 4 commits into
MetOffice:mainfrom
ScottWales:time-bounds

Conversation

@ScottWales
Copy link
Copy Markdown
Contributor

@ScottWales Scott Wales (ScottWales) commented May 10, 2026

Guessing time bounds for instantaneous fields causes problems for METplus and Verpy when working with CSET-processed files.

Fixes #2101

Contribution checklist

Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.

  • Documentation has been updated to reflect change.
  • New code has tests, and affected old tests have been updated.
  • All tests and CI checks pass.
  • Ensured the pull request title is descriptive.
  • Ensure rose-suite.conf.example has been updated if new diagnostic added.
  • Conda lock files have been updated if dependencies have changed.
  • Attributed any Generative AI, such as GitHub Copilot, used in this PR.
  • Marked the PR as ready to review.

@ScottWales
Copy link
Copy Markdown
Contributor Author

There are some failing tests with this change:

  • tests/operators/test_collapse.py::test_collapse_by_validity_time_no_common_points, tests/operators/test_misc.py::test_difference_no_time_coord, tests/operators/test_imageprocessing.py::test_SSIM_no_time_coord
    Tests are extracting cubes using times not present in the file - file has [2022-09-21 03:00:00, 2022-09-21 04:00:00, 2022-09-21 05:00:00], tests are trying to load times on the half hour. An error in the test code? Would be simple to change the loads to be on the hour instead.
  • tests/operators/test_plot.py::test_get_start_end_strings_bounds
    Expected change, bounds are no longer being created for the instantaneous time. The function could be updated to guess the time bounds to retain the previous behaviour, or the test values updated to reflect that these bounds are from an instantaneous field.

James Frost (@jfrost-mo), JorgeBornemann (@JorgeBornemann) any preferences on how these are resolved?

@JorgeBornemann
Copy link
Copy Markdown
Collaborator

* **tests/operators/test_collapse.py::test_collapse_by_validity_time_no_common_points**, **tests/operators/test_misc.py::test_difference_no_time_coord**, **tests/operators/test_imageprocessing.py::test_SSIM_no_time_coord**
  Tests are extracting cubes using times not present in the file - file has `[2022-09-21 03:00:00, 2022-09-21 04:00:00, 2022-09-21 05:00:00]`, tests are trying to load times on the half hour. An error in the test code? Would be simple to change the loads to be on the hour instead.

Maybe duplicating the test? one that loads cubes that correctly have bounds in time and reads whatever times are required and another test that loads cubes that don't have bounds and reads times on the instant times?

* **tests/operators/test_plot.py::test_get_start_end_strings_bounds**
  Expected change, bounds are no longer being created for the instantaneous time. The function could be updated to guess the time bounds to retain the previous behaviour, or the test values updated to reflect that these bounds are from an instantaneous field> 

I would prefer to update the test values, but we may need to retain the coverage of the initial test, looking into cubes that require bounds, so perhaps also duplicating the test?

@ScottWales
Copy link
Copy Markdown
Contributor Author

Thanks JorgeBornemann (@JorgeBornemann). I have fixed up the first set by correcting the times in the test and adding a new test in test_reads that makes sure fields are loaded when the time constraint is inside the time bounds.

For the plotting tests there's adjacent tests for fields with and without bounds, so I've moved things around so that each of these is getting the correct cubes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The changes are sound. For me they are good to implement but, as they touch a very generic operator I leave the approval to James Frost (@jfrost-mo) who is more familiar whit it.

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.

Don't guess time bounds for instantaneous fields

2 participants