-
Notifications
You must be signed in to change notification settings - Fork 288
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 decimal rounding to geostationary area utilities for better consistency #2573
base: main
Are you sure you want to change the base?
Conversation
I saw AGRI reader failures so tested with a real data case. Here's the old areas:
New:
So now these are consistent as well. |
I had one SEVIRI HRIT case on my local machine. Old:
New:
So 1.5 meters for the HRV area I think that is? And 0.03 meters for the 3km bands. I do notice that in the new version the HRV area is actually further shifted from the 3km area than it was in the old one. |
I'd like @ameraner to confirm for the seviri part. |
Unfortuately, I'm not too happy about this, tbh. It causes some issues:
We actually have a similar issue as the one you're trying to solve in the case of the FCI grids, where different resolutions have slightly (in the order of millimeters) different area extents only because of numerical precision constraints (multiplying Bottom line, I'm not against your changes in general, but I would also endorse @sfinkens idea in slack to make this optional, and then let the developer of each reader decide how the computation should be handled case-by-case, considering different format issues etc. of the specific instrument. |
A suggestion from @sfinkens on slack was to make the rounding optional. I think this can be doable for this particular utility function and only be used for AHI at the moment. I don't think it should be a reader-level kwarg. To me it is more a problem that AHI isn't consistent and the unfortunate side effect of this is that AHI passes its inconsistent parameters to a shared utility function. Rounding these parameters produced way too much of a difference if I recall correctly so the rounding went into the utility function. I'll see if I have time to move to this optional rounding keyword argument solution. |
Oops I didn't see your big comment @ameraner until I had already posted my previous one. As I eluded to in my comment, I am OK handling this in a reader by reader basis. I understand (and feel similarly about) that putting a "random" rounding operation in the middle of the math on calculating extents just seems wrong and like cheating just to get the answer I want. As for why I want these to be consistent: I think my main motivation is the theoretical extents of the data/instrument. For ABI (and AHI and similar instruments) the footprint of a coarse resolution pixel is meant to (at least my understanding) align perfectly with the footprints of multiple finer resolution pixels. For example, 4 500m pixels fit into one 1km pixel. That is, with the same exact extents. When I updated the ABI L1b reader to use some level of rounding it was to match this theoretical expectation: the pixels should be aligned between resolutions and the center point of the geostationary reference disk is perfectly half way between the edges of the image. For ABI this was a decently easy argument to make because for whatever reason the X and Y coordinate variables were given 32-bit float scale factors and offsets which are insufficient to accurately describe the coordinates across a full disk grid using the advertised operations (data * factor + offset). For AHI HSD, it might not be that easy of an excuse. As for my concerns: I worry that equality checks and boundary operations will fail between resolutions when put to the test. For the native resampler and maybe others we worked around this iirc by checking of the extents of two area definitions were "close enough". Additionally, having them be inconsistent means that per-pixel resolutions are different too. And that doing any operation to convert from one area to another will strongly depend on which area you use as a starting point. For example, producing a 4km reduced resolution area definition will have different extents depending on if we start with 500m or 1km or 2km versions of the instrument's area. And if we went from 500m to compute 1km (say in an aggregation step), then there is a very good chance that the aggregated 500m data on a 1km area will not be the same as the 1km area of the data loaded from the data files. |
Ok I think the tests should be updated now, but the more and more I work on this the less pleased I am. I never disagreed with you guys (@simonrp84 and @ameraner) I just want consistent extents. I just don't know enough about the math I think to know what's the most correct way to get what I want (rounding an offset feels a lot better than rounding an intermediate radian angle). |
Codecov Report
@@ Coverage Diff @@
## main #2573 +/- ##
=======================================
Coverage 94.92% 94.92%
=======================================
Files 352 352
Lines 51225 51233 +8
=======================================
+ Hits 48624 48632 +8
Misses 2601 2601
Flags with carried forward coverage won't be shown. Click here to find out more.
|
3ea3ef5
to
2e12275
Compare
Pull Request Test Coverage Report for Build 6461251984
💛 - Coveralls |
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.
LGTM, just an optional improvement suggestion for the docstrings.
And noted regarding your rationale, makes sense. We'll consider doing the same as for FCI, as described above.
round: Round values by 7 decimal places to avoid inconsistencies | ||
with floating point calculations. |
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.
Just a suggestion, to improve clearness
round: Round values by 7 decimal places to avoid inconsistencies | |
with floating point calculations. | |
round: Round intermediate area extent angle values by 7 decimal places | |
to avoid inconsistencies with floating point calculations. |
(the same to the get_area_extent
docstring).
I also notice now that the docstring here is wrong, since the input values are in deg
instead of the documented m
.
@@ -41,7 +41,7 @@ def get_xy_from_linecol(line, col, offsets, factors): | |||
return x__, y__ | |||
|
|||
|
|||
def make_ext(ll_x, ur_x, ll_y, ur_y, h): | |||
def make_ext(ll_x, ur_x, ll_y, ur_y, h, round=False): |
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.
What about passing the number of decimals? Like
def make_ext(..., round_decimals=None):
...
my_ext = make_ext(..., round_decimals=4)
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.
Good idea!
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.
I'm not a huge fan of this because the rounding is technically in an arbitrary location. Putting the rounding somewhere else results in different results. The generic round
kwarg was meant to convey "don't worry about it, we'll round where and how we have to".
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.
Ok, so in this case how about calling it something more along the lines of align_corners
or eg snap_extents_to_closest_meter
? Because that's the goal, right?
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.
Eh I wish it was as simple as getting to the nearest meter, but in tests that's not what is going on. In ABI the pixel resolution is not a nice round metered number, it is something like 1015.9 meters for the 1km data (this is off the top of my head, no idea if that's accurate). For AHI, I'm not sure it follows the same assumptions/design as ABI down to this level of detail. @simonrp84 might know more details, but I know I've had trouble googling it in the past. If we had a theoretical goal for what the extents would be then we could be more specific about numbers being passed in: the pixels should be 1002.15 meters so the left extent should be (num_cols / 2 * -1002.15).
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.
So I never did change the keyword argument to take an integer number of decimal places and I don't think we ever decided on a name for that keyword argument that I considered accurate. As mentioned above I'm not rounding to the nearest/closest meter and I'm not necessarily aligning corners although that is kind of the goal. I think there is conflicting hopes for this functionality.
On one side we don't want a generic non-flexible hardcoded rounding that may be sensor/reader specific. On the other side of the argument, we might not want developers to have to think too hard about all this decimal/rounding number "fudging" so a generic "do the rounding" argument would be nice...but maybe we do want developers to have to think about this even if it is hacky.
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.
Looks good, I think it's a good idea to let the user provide the number of decimals as @sfinkens is proposing.
Should we merge this? It looks like giving more flexibility to the rounding might not be so good after all, so I think we can merge this as soon as the conflicts are resolved. |
I actually forgot about this. I think the conversation on slack led me to think that this wasn't a good idea. Maybe I'm misremembering. |
I think it's useful |
@djhoese I'll let you fix the conflicts and press the merge button |
@mraspaud as mentioned in the above comments, my understanding was that as-is this isn't good enough. |
Ok, should we close this for now then? Or convert it to draft? |
Converted to draft. |
As discussed a little in #2560, the AHI HSD reader (which uses the geos utilities) produces inconsistent area extents between resolutions (usually not between time steps). In the ABI readers I put extra work into rounding the calculations so that they produced the same output between resolutions. I did a quick test at a few points in the geos utilities to see if something similar could be done for AHI.
The numbers going in to these calculations are all pretty simple and float friendly (ex.
0.5
), but then there is a2**16
thrown in there, then I tried rounding some things and it didn't really change much. It wasn't until I rounded extents near thenp.deg2rad
that things started being a little more consistent. For AHI I saw the best results rounding afternp.deg2rad
and found that rounding to 7 decimal places was still producing consistent results. What I don't like is changing the number of decimal places can change the results of the Y dimension 10s of meters for each segment.Anyway, here's the results of this PR:
And for the record, here's what
main
produces:AUTHORS.md
if not there already