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

Allow real-world co-ordinates to be specified in single-point timeseries. #943

Merged
merged 9 commits into from
Feb 10, 2025

Conversation

cehalliwell
Copy link
Contributor

@cehalliwell cehalliwell commented Nov 20, 2024

When selecting longitude and latitude points to create a single point timeseries, the user now has the option to select whether the chosen co-ordinates are on the model's rotated grid or the real-world grid. This branch adds the real-world option. The new code includes an operator that transforms a point from the real-world co-ordinates to the model's rotated grid. This may benefit future CSET developments.

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 Nov 20, 2024

Coverage

@cehalliwell
Copy link
Contributor Author

ChatGPT was used for guidance only. No code suggestions from ChatGPT were used in this pull request.

Copy link
Contributor

@daflack daflack left a comment

Choose a reason for hiding this comment

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

Science Review:

Really useful addition from science and UX perspectives. It was great to have updated the recipe so that others can see how to update other relevant recipes (e.g. transects). I've raised one point for you to think about to make it easier for spotting errors, otherwise from the science side it looks good to me. Technical review required before merger.

src/CSET/operators/regrid.py Outdated Show resolved Hide resolved
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.

Looking good. Aside from some tweaks, the big question I have is whether we want to make this configurable at all.

It would add rather a lot of complexity if everywhere we use user provided coordinates we have to figure out what type they are, so it would be nice to support just one type.

Are there compelling use cases for keeping rotated pole coordinates?

cset-workflow/meta/diagnostics/rose-meta.conf Show resolved Hide resolved
src/CSET/operators/regrid.py Outdated Show resolved Hide resolved
src/CSET/operators/regrid.py Show resolved Hide resolved
src/CSET/operators/regrid.py Outdated Show resolved Hide resolved
Copy link
Contributor

@daflack daflack left a comment

Choose a reason for hiding this comment

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

Happy from a science perspective.

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.

Looking good from a technical perspective.

There was a minor indentation issue, but I've fixed it myself, and am happy to approve.

This PR is now ready to merge, so go ahead and press the green "Merge pull request" button at the bottom of the page to merge it to trunk.

src/CSET/operators/regrid.py Outdated Show resolved Hide resolved
@JKPShonk JKPShonk merged commit ec7e5c7 into main Feb 10, 2025
8 checks passed
@JKPShonk JKPShonk deleted the 820_select_true_grid_coordinates branch February 10, 2025 16:48
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.

Select points with real world coordinates, rather than grid coordinates
4 participants