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

ADD: Utility to subset a radar volume for the column above a location #1113

Merged
merged 4 commits into from
Apr 21, 2022

Conversation

jrobrien91
Copy link
Collaborator

Given the lat/lon of the location, return a dictionary containing all radar object fields for the column above the location, as well as, x,y,z gates for the column. Tested with ppi (nexrad) and rhi (csapr) scans.

given the lat/lon of the location. Return is a dictionary containing all radar object fields for the column above the location, as well as, x,y,z gates for the column. Tested with ppi (nexrad) and rhi (csapr) scans
@zssherman
Copy link
Collaborator

zssherman commented Apr 15, 2022

Thanks for the PR @jrobrien91 ! I had some minor suggestions, did you plan on including unittests as well?

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.

Thanks for taking the time to add this new functionality @jrobrien91 !! I added a few comments - let me know if you have any questions!

Also, I agree with @zssherman here - unit tests with this PR would be great!

from ..core.transforms import antenna_vectors_to_cartesian


def sphere_distance(rad_lat, tar_lat, rad_lon, tar_lon):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you spell out radar_latitude, target_latitude... I think this would make it more clear to the end user in terms of the API.

check_lat(tar_lat)
check_lon(rad_lon)
check_lon(tar_lon)
# convert latitude/longitudes to radians
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think spaces would help in between each of these sections

return distance


def for_azimuth(rad_lat, tar_lat, rad_lon, tar_lon):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as the previous comment - more clear variable names would help

check_lat(tar_lat)
check_lon(rad_lon)
check_lon(tar_lon)
# convert latitude/longitudes to radians
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above in terms of separating sections

return azimuth


def get_fields_latlon(radar, lat, lon):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you make this get_field_location? Ans spell out latitude/longitude?

column = dict(meta)
column.update(moment)

return column
Copy link
Collaborator

Choose a reason for hiding this comment

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

this would be a good point to convert to either an Xarray.Dataset or a pyart.core.Radar object...

Comment on lines 205 to 212
for i in range(len(ray)):
tar_gate = np.argmin(abs(rhidis[i, 1:] - (dis)))
for key in moment:
moment[key].append(radar.fields[key]['data'][ray[i], tar_gate])
# fill the meta dictionary for further analysis
meta['xgate'].append(rhi_x[i, tar_gate])
meta['ygate'].append(rhi_y[i, tar_gate])
meta['zgate'].append(rhi_z[i, tar_gate])
Copy link
Collaborator

Choose a reason for hiding this comment

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

How long does this usually take using this for loop?

Copy link
Collaborator Author

@jrobrien91 jrobrien91 Apr 18, 2022

Choose a reason for hiding this comment

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

Working through the other comments and changing output to an Xarray Dataset, but the timing is:
with a NEXRAD Level 2 file - 0.0005 sec
with a CSAPR-2 RHI file - 0.0006 sec

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome! Thanks!

if (isinstance(lat, int) or isinstance(lat, float) or
isinstance(lat, np.floating)) is True:
if (lat <= -90) or (lat >= 90):
raise ValueError("latitude not valid")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a more descriptive error here? Ex. latitude not between -90 to 90, need to convert to values between -90 and 180

if (lat <= -90) or (lat >= 90):
raise ValueError("latitude not valid")
else:
raise TypeError("latitude type not valid")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here - maybe mention that lat needs to be an int or float? Does this support a list of lats? Maybe add a check for that.

"""
Function to check if input latitude is valid for type and value
"""
if (isinstance(lon, int) or isinstance(lon, float) or
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as the lat function - and change these to check_lat, check_lon

Still working on transfering output to an xarry object
…nclude changing the output of

get_field_location form a dictionary to xarray Datatset.
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.

Looks fantastic! Thanks for all your hard work on this @jrobrien91

@mgrover1 mgrover1 merged commit d50ee0a into ARM-DOE:main Apr 21, 2022
@jrobrien91 jrobrien91 deleted the ldquants branch April 22, 2022 14:30
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.

3 participants