search

ARM-DOE / pyart

The Python-ARM Radar Toolkit. A data model driven interactive toolkit for working with weather radar data.
https://arm-doe.github.io/pyart/
Other
516 stars 268 forks source link

No method for specifying the altitude of a grid in the map_to_grid function #138

Closed jjhelmus closed 9 years ago

jjhelmus commented 10 years ago

The map_to_grid function does not provide a method for specifying the starting altitude of a grid. The default is to set it to the altitude of the first radar which can then be adjusted by setting the limits of the z grid_limits. This is far from ideal.

gamaanderson commented 9 years ago

I would like to revive this topic, since it is not really clear to me and there may to be some inconsistence in file io.grid_mapper.py. Function map_to_grid consider in lines 399 grid origin altitude to be always zero, but the reference is not clear, since it does not correct radar.altitude when calculating Cartesian coords (line 455). There is also a comment in line 445 On the other hand, function grid_from_radars in line 136 consider grid origin to be the first radar altitude.

I believe we should clarify this once for all and add this information in the function description. As for me I would prefer altitude to be (always) absolute (meters above sea level).

jjhelmus commented 9 years ago

I agree that the user should be able to specify an altitude above sea level for the starting level of the grid and that the algorithm should correct for the difference between this altitude and the altitudes of the Radar objects which are being gridded. I still think that the default value should be the altitude of the first radar.

A bit more explanation of what the code current does. In the Grid classes axes attribute there are two dictioaries, alt and z_disp which record the altitude of the origin of the grid and the displacement from this origin. To get the altitude above mean sea level for each point in the grid one must add the value found in alt dictionary to the values found in z_disp dictionary. This is why line 136 in the grid_from_radars function sets alt to the altitude of the first radar, this is considered the origin of the grid.

In the gridding algorithm the altitudes of all the radars are assumed to be the same, this assumption is not correct and any displacements in altitude between the radars should be taken into account which is why there is a XXX comment on line 445. If/when the option to specify the altitude of the radar is added then offsets for all radars altitudes to this value should be calculated and the z gate locations should be corrected in line 455 in the same manner as is done for the y and x gate locations.

gamaanderson commented 9 years ago

Being that the case, I believe is just a question of implementing. Unfortunately, because of the z,y,x order, transforming the grid_origin in 3-truplet has the possibility of creating bugs. Nevertheless, I think this should be the way of doing it. How are you dealing with somethings like this, updates that may create bugs in user-scripts? How connected are you to your users?

scollis commented 9 years ago

FYI The z,x,y order is there to mirror CF standards

On 4/23/15 11:54 AM, Anderson wrote:

Being that the case, I believe is just a question of implementing. Unfortunately, because of the z,y,x order, transforming the grid_origin in 3-truplet has the possibility of creating bugs. Nevertheless, I think this should be the way of doing it. How are you dealing with somethings like this, updates that may create bugs in user-scripts? How connected are you to your users?

— Reply to this email directly or view it on GitHub https://github.com/ARM-DOE/pyart/issues/138#issuecomment-95652272.

I ride for Parkinsons research http://www.events.org/sponsorship.aspx?id=51573

jjhelmus commented 9 years ago

Yes, the question is only implementation. The grid_origin parameter could be used to specify the altitude, latitude and longitude but this would break backwards compatibility and is a bit awkward as two of the values are in degrees and the other is in meters (or km). I'm be more in favor of the more verbose grid_origin_alt which would be either None (to use the altitude of the first radar) or a float. This does not break backwards compatibility (although the bug with multiple radars should be fixed which will results in different results) and does not mix units in a tuple.

gamaanderson commented 9 years ago

Ok, I did a quick implementation (PR #284).

I don't find mixing units awkward at all, in the end latlon is degreeN and degreeE. They are not exactly the same thing, nor are independent, but two components of a projection; but I do understand your point.

Also, since Scott raise the point of CF standard, I would like to point out that using the variable 'alt' as offset for 'z_disp' does not seem for me CF standard, that should be an attribute, even in the "units" attribute. As for 'lat' and 'lon' that argument shall not apply, since they are not offsets, but rather describing the Projection used. By the way, CF has it own way of describing a Projection, but as that is file oriented, I not sure if it is usable here.

jjhelmus commented 9 years ago

The Grid class does not do a good job following the CF standards, the layout of the class is historical and should be replaced. I've been looking as basing the class on the Gridspec standard, but I have not read enough of the standard to see if it is appropriate. I'll open a new issue for replacing the Grid class.

jjhelmus commented 9 years ago

Opened #285 for continued discussion on new Grid class