pytroll / pyresample

Geospatial image resampling in Python
http://pyresample.readthedocs.org
GNU Lesser General Public License v3.0
346 stars 94 forks source link

Fix geostationary bbox having inf values #493

Closed mraspaud closed 1 year ago

mraspaud commented 1 year ago

As described in #492 , this PR fixes problems in geostationary bbox computation

codecov[bot] commented 1 year ago

Codecov Report

Merging #493 (b20f66c) into main (8393134) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #493   +/-   ##
=======================================
  Coverage   94.32%   94.33%           
=======================================
  Files          74       74           
  Lines       12889    12947   +58     
=======================================
+ Hits        12158    12214   +56     
- Misses        731      733    +2     
Flag Coverage Δ
unittests 94.33% <100.00%> (+<0.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyresample/geometry.py 87.46% <100.00%> (-0.05%) :arrow_down:
pyresample/gradient/__init__.py 95.14% <100.00%> (ø)
pyresample/test/test_geometry.py 99.55% <100.00%> (+0.01%) :arrow_up:
pyresample/test/test_gradient.py 98.98% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

coveralls commented 1 year ago

Coverage Status

Coverage: 93.862% (+0.01%) from 93.85% when pulling b20f66cb48f1edf17f66417c94dbf5d1a2644f52 on mraspaud:fix-geos-bb into 8393134323e3aeba15a1a23c396c0e0208ba2970 on pytroll:main.

mraspaud commented 1 year ago

@ghiggi I would just like to confirm with you that this is the correct fix. I looks like that for pyresample <=1.25.0, having np.inf values in spherical polygon was ok for doing intersections, while now only np.nan is working. Is it supposed to be like this, and is this documented somewhere?

gerritholl commented 1 year ago

Have you tried if this by any chance also fixes #393?

mraspaud commented 1 year ago

Have you tried if this by any chance also fixes #393?

I did and it doesn't unfortunately.

mraspaud commented 1 year ago

Alright, I think this is ready for re-review @djhoese @ghiggi.

The problem was quite simple, but the solution is not as easy as I would have liked. Some explanation: To get the geostationary bounding box, that is a list of coordinates around the side of the earth disk, we draw a sampled ellipse that is the size of the earth. This was working without problem for the full earth disk, and still is, I have just refactors things a little and added a test. The problem appeared when we just had an area describing a portion of the earth that did not cover the equator. In that case, the simple clipping we did to "cut" the disk was giving (unneeded) points outside the valid disk. To prevent this, the easiest solution I could think of was to actually take the full earth bounding box and intersect it with the rectangle defined with the area extent of the area at hand. The obvious choice to do this, since we are in projection coordinates (no poles, no date shift line) is shapely. So I used shapely here, because I didn't want to reinvent the wheel. I hope you are ok with that. Question is now if we make shapely a hard dependency or not...

djhoese commented 1 year ago

Looks like the wheel building is failing on 32-bit linux because it has to build shapely 2.x since shapely doesn't provide 32-bit wheels. This says to me we should get rid of 32-bit. I think we're already requiring ourselves to build numpy on 32-bit and if the maintainers of these packages aren't going to go through the work of building on 32-bit I don't think we can expect our selves to do it.

mraspaud commented 1 year ago

Looks like the wheel building is failing on 32-bit linux because it has to build shapely 2.x since shapely doesn't provide 32-bit wheels. This says to me we should get rid of 32-bit. I think we're already requiring ourselves to build numpy on 32-bit and if the maintainers of these packages aren't going to go through the work of building on 32-bit I don't think we can expect our selves to do it.

I removed the 32 bit build now.