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

Fix unit-handling in azimuth_range_to_lat_lon #1970

Closed
dopplershift opened this issue Jul 15, 2021 · 3 comments · Fixed by #1977
Closed

Fix unit-handling in azimuth_range_to_lat_lon #1970

dopplershift opened this issue Jul 15, 2021 · 3 comments · Fixed by #1977
Labels
Area: Calc Pertains to calculations Type: Bug Something is not working like it should
Milestone

Comments

@dopplershift
Copy link
Member

azimuth_range_to_lat_lon currently says:

def azimuth_range_to_lat_lon(azimuths, ranges, center_lon, center_lat, geod=None):
"""Convert azimuth and range locations in a polar coordinate system to lat/lon coordinates.
Pole refers to the origin of the coordinate system.
Parameters
----------
azimuths : array_like
array of azimuths defining the grid. If not a `pint.Quantity`,
assumed to be in degrees.
ranges : array_like
array of range distances from the pole. Typically in meters.

This is problematic because:

  1. Contrary to the docs, we don't actually handle non-degrees
  2. We suggest meters, but really it is required to be in meters when passing to PyProj. We're a unit-aware library, the whole point is to be able to help users.

What we should be doing is:

  1. Convert to degrees as necessary using Pint for azimuth
  2. Make range be like azimuth and say e.g. "Must be in meters unless a Quantity". Then convert to meters as possible when sending to PyProj.

An option to consider for 2.0 is to require units, which for some reason isn't being done here.

@dopplershift dopplershift added Area: Calc Pertains to calculations Type: Bug Something is not working like it should labels Jul 15, 2021
@dopplershift dopplershift added this to the 1.1.0 milestone Jul 15, 2021
@dopplershift
Copy link
Member Author

Putting as a nice to have for 1.1, but not a blocker by any means.

@kgoebber
Copy link
Collaborator

I will take the gitblame for the current implementation. These are reasonable (and needed) changes to make this function better and align it with our unit aware functionality. I'll hopefully attempt a PR for 1.1, but agree that we shouldn't hold our release up for this issue.

@dopplershift
Copy link
Member Author

This comes from an e-support ticket we received where there was confusion passing in range in km from our radar code--no idea if that code is properly assigning units, so the changes here may or may not be sufficient to completely avoid the problem.

Also this received my famously thorough code review that clearly eliminated these problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Calc Pertains to calculations Type: Bug Something is not working like it should
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants