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

ENH: add spatial_ref with pyproj when georeferencing, add/adapt methods/tests #87

Merged
merged 2 commits into from Feb 22, 2023

Conversation

kmuehlbauer
Copy link
Collaborator

@kmuehlbauer kmuehlbauer commented Feb 15, 2023

This adds spatial_ref to the datasets when georeferencing. That way users can easily wrap this into cartopy and use this for georeferenced plotting.

  • tests added/adapted
  • added example to plot-ppi.ipynb notebook
  • takes site coordinates into account (altitude) for georeferencing
  • site coordinates are in default model Dataset (Locarno Monti, MeteoSvizzera coords, birthplace of xradar)

Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

One small suggestion here - can we call it add_crs instead of write_crs? Also, by default, can we call this when "georeference" the data?

@@ -95,6 +98,10 @@ def get_x_y_z(ds, earth_radius=6371000, effective_radius_fraction=None):
.. [1] Doviak and Zrnic, Doppler Radar and Weather Observations, Second
Edition, 1993, p. 21.
"""
if earth_radius is None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mgrover1 Actually it is written in any case, when georeferenced. So the user doesn't have to do anything, beside georeferencing.

@kmuehlbauer
Copy link
Collaborator Author

@mgrover1 I've refactored the naming per your suggestion.

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #87 (87537e7) into main (3435b08) will decrease coverage by 0.25%.
The diff coverage is 75.40%.

@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
- Coverage   88.00%   87.75%   -0.25%     
==========================================
  Files          18       19       +1     
  Lines        3227     3284      +57     
==========================================
+ Hits         2840     2882      +42     
- Misses        387      402      +15     
Flag Coverage Δ
unittests 87.75% <75.40%> (-0.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
xradar/accessors.py 59.32% <37.50%> (-8.86%) ⬇️
xradar/georeference/projection.py 82.75% <82.75%> (ø)
xradar/georeference/__init__.py 100.00% <100.00%> (ø)
xradar/georeference/transforms.py 100.00% <100.00%> (ø)
xradar/model.py 95.50% <100.00%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kmuehlbauer
Copy link
Collaborator Author

@mgrover1 I'm ironing out the CI/doc issues. We can let this sit another while. No rush for now to get this in.

@kmuehlbauer
Copy link
Collaborator Author

This is the direct link to the docs with a plotting example:

https://openradar-docs--87.org.readthedocs.build/projects/xradar/en/87/notebooks/plot-ppi.html#Plot-PPI-on-map-with-cartopy

@kmuehlbauer
Copy link
Collaborator Author

@mgrover1 Heads-up, I've found some problems with the docs which are slightly related to this PR and I'll add that here. I'll also add some more code to model.py for the sitecoords.

@kmuehlbauer
Copy link
Collaborator Author

I've also found an issue that get_x_y_z didn't take the radar site altitude into account, I've fixed this too.

Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Great!

@kmuehlbauer
Copy link
Collaborator Author

@mgrover1 I'll merge tomorrow, if no-one beats me to it.

…ethods, take site altitude into account, add Locarno MeteoSvizzera site coordinates to model, fix tests, fix docs
@kmuehlbauer
Copy link
Collaborator Author

I've rearranged/squashed the commits into two for rebase-merge. Getting this when CI is green.

@kmuehlbauer kmuehlbauer merged commit 50887fb into openradar:main Feb 22, 2023
@kmuehlbauer kmuehlbauer deleted the pyproj-crs branch February 22, 2023 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants