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 LFRic time coordinate metadata #1117

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix LFRic time coordinate metadata #1117

wants to merge 5 commits into from

Conversation

jwarner8
Copy link
Contributor

@jwarner8 jwarner8 commented Feb 4, 2025

Fixes LFRic coordinate metadata, by adding forecast_period and forecast_reference_time, summarised in #1113

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.
  • 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.

Copy link
Contributor

github-actions bot commented Feb 4, 2025

Coverage

Copy link
Member

@jfrost-mo jfrost-mo left a comment

Choose a reason for hiding this comment

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

A small observation. Let me know when you want a proper review on this.

src/CSET/operators/read.py Outdated Show resolved Hide resolved
@jwarner8
Copy link
Contributor Author

jwarner8 commented Feb 6, 2025

A small observation. Let me know when you want a proper review on this.

THanks, did you say you were working off this branch for a more general cube time merge fix?

jwarner8 and others added 3 commits February 6, 2025 15:32
@jwarner8 jwarner8 self-assigned this Feb 6, 2025
@jwarner8 jwarner8 requested a review from jfrost-mo February 6, 2025 15:39
@jfrost-mo
Copy link
Member

I'll get this reviewed and merged first.

Copy link
Member

@jfrost-mo jfrost-mo left a comment

Choose a reason for hiding this comment

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

Looks good to me. The comments are minor formatting things, and I'm happy for you to merge after addressing them.

src/CSET/operators/read.py Outdated Show resolved Hide resolved
src/CSET/operators/read.py Show resolved Hide resolved
src/CSET/operators/read.py Show resolved Hide resolved
@jfrost-mo jfrost-mo self-requested a review February 7, 2025 14:54
Copy link
Member

@jfrost-mo jfrost-mo left a comment

Choose a reason for hiding this comment

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

Some additional tests are needed.

Co-authored-by: James Frost <james.frost@metoffice.gov.uk>
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.

2 participants