-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Allow real-world co-ordinates to be specified in single-point timeseries. #943
base: main
Are you sure you want to change the base?
Conversation
…d single point is on the real world grid or the rotated grid used by the input data
mapping this point correctly onto the rotated grid
…t answer for the test cube.
ChatGPT was used for guidance only. No code suggestions from ChatGPT were used in this pull request. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this 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?
[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 |
There was a problem hiding this comment.
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.
"""Transform a selected point in longitude and latitude in ... | ||
|
||
the real world to the corresponding point on the rotated grid | ||
of a cube. |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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?
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.