-
Notifications
You must be signed in to change notification settings - Fork 5
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
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.
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?
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.
Happy from a science perspective.
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 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.
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.