Closed mraspaud closed 1 year ago
@ameraner you might want to have a look at this also
Merging #516 (44af636) into main (2a6d769) will decrease coverage by
0.10%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## main #516 +/- ##
==========================================
- Coverage 94.34% 94.25% -0.10%
==========================================
Files 82 82
Lines 13026 13030 +4
==========================================
- Hits 12290 12282 -8
- Misses 736 748 +12
Flag | Coverage Δ | |
---|---|---|
unittests | 94.25% <100.00%> (-0.10%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
pyresample/geometry.py | 87.64% <100.00%> (+0.05%) |
:arrow_up: |
pyresample/test/test_geometry/test_area.py | 99.21% <100.00%> (+<0.01%) |
:arrow_up: |
If I'm not mistaken this will mean that reduce_data
in Satpy will now cause a performance hit for all LEO data right? For some reason I thought reduce_data depended on this failing to not have reduce_data be performed on LEO/SwathDefinition data.
Yep, this NotImplementedError handling here:
If I'm not mistaken this will mean that
reduce_data
in Satpy will now cause a performance hit for all LEO data right? For some reason I thought reduce_data depended on this failing to not have reduce_data be performed on LEO/SwathDefinition data.
So maybe we should check that the it is an areadef not swathdef here? or should that be done in satpy to keeps things cleaner here?
Oh I'm wrong. There is an AreaDefinition check at the top of the method. This is probably good then.
@mraspaud: FYI there are several edge cases where this method currently fails:
If you implement get_array_indices_from_lonlat
method also for SwathDefinition
, then the method would work also when area_to_cover
is SwathDefinition
;)
Oh I'm wrong. There is an AreaDefinition check at the top of the method. This is probably good then.
I can't see it, could you point me to it please?
Hhhmmm maybe I should have merged this before my big config PR. Sorry, but I've caused some conflicts.
Fixed.
@djhoese please review my two last commits to see if you agree with the refactoring I made.
This refactoring took me some time because I found a bug/inconsistency in the usage of the frequency
argument in AreaDefBoundary
and the boundary
method of area defintion: in the first case it means the step we use to determine the length of the boundary, in the second case it is the length of the boundary itself. I'm working on a fix now (which will be another PR).
I remember I noticed such inconsistencies already a while ago. For GEO areas is the total number of vertices, while for swath and other areas is the number of steps on each boundary side right? I personally would really like to have this as the number of vertices per boundary side, and allow to specify a tuple (y,x) or (x,y) instead of a single value for all dimensions.
I also remember somewhat that quite ago, by using the step approach, we were discarding boundary corners values, but I think this was fixed by one of mine or your PR @mraspaud.
@ghiggi I think both approaches are valid, the usage just depends on what the user needs at that point.
in https://github.com/pytroll/pyresample/pull/526 I have now just replaced frequency
with bbox_shape
as a more accurate name. I don't change any functionality or behaviour though, so allowing to pass a tuple for example isn't supported (yet?)
which then reaches a PendingDeprecationWarning
Indeed, this is now fixed.
@mraspaud I think I understand the last 2 commits. I assume the main motivation was to:
So if that is true then :+1: from me.
git diff origin/main **/*py | flake8 --diff