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

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

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.

Parameters
----------
cube: Cube
An iris cube of the data to regrid. As a minimum, it needs to be 2D with
Copy link
Contributor

@daflack daflack Dec 3, 2024

Choose a reason for hiding this comment

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

Is it worth some form of check for dimensions, and an error message raised if the minimum dimensions are not reached?

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?

Comment on lines +74 to +81
[template variables=LATLON_IN_TYPE]
ns=Diagnostics/Quicklook
description=Method used to map model data onto selected gridpoints.
help=This switch indicates whether the selected latitude longitude coordinates are on the real world grid or
the rotated grid specified by the input data.
values="realworld", "rotated"
compulsory=true
sort-key=0surface6
Copy link
Member

Choose a reason for hiding this comment

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

In what cases would we not want to use real world coordinates? I'm wondering if we need this to be configurable, or if we should just use them all the time.

Comment on lines +334 to +337
"""Transform a selected point in longitude and latitude in ...

the real world to the corresponding point on the rotated grid
of a cube.
Copy link
Member

Choose a reason for hiding this comment

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

Its best not to break the first line.

Generally the first line acts as a subject line, or a very concise summary. Then you can go into a bit more detail in however many paragraphs you need after the line break.

import cartopy.crs as ccrs

rot_pole = cube.coord_system().as_cartopy_crs()
true_grid = ccrs.Geodetic()
Copy link
Member

Choose a reason for hiding this comment

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

Its probably fine, but is this using a sensible datum?

@@ -202,6 +202,7 @@ def regrid_to_single_point(
cube: iris.cube.Cube,
lat_pt: float,
lon_pt: float,
latlon_in_type: str,
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to set the default of this to "rotated" so existing code continues to work?

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