Add 'antimeridian_mode' to DynamicAreaDefinition #431

djhoese · 2022-05-06T20:32:32Z

Closes #428. See #428 for more details about the possible cases here.

pyresample/geometry.py

codecov · 2022-06-26T19:48:35Z

Codecov Report

Merging #431 (79edf86) into main (1b14698) will increase coverage by 0.26%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main     #431      +/-   ##
==========================================
+ Coverage   93.95%   94.21%   +0.26%     
==========================================
  Files          65       68       +3     
  Lines       11252    12346    +1094     
==========================================
+ Hits        10572    11632    +1060     
- Misses        680      714      +34

Flag	Coverage Δ
unittests	`94.21% <91.66%> (+0.26%)`	⬆️

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

Impacted Files	Coverage Δ
pyresample/geometry.py	`87.12% <88.31%> (+0.07%)`	⬆️
pyresample/test/test_geometry.py	`99.51% <100.00%> (+0.03%)`	⬆️
pyresample/utils/proj4.py	`100.00% <100.00%> (ø)`
pyresample/gradient/__init__.py	`95.14% <0.00%> (-2.81%)`	⬇️
pyresample/test/utils.py	`74.76% <0.00%> (-0.94%)`	⬇️
pyresample/bilinear/xarr.py	`92.90% <0.00%> (-0.11%)`	⬇️
pyresample/area_config.py	`90.55% <0.00%> (ø)`
pyresample/test/test_bucket.py	`100.00% <0.00%> (ø)`
pyresample/test/test_bilinear.py	`100.00% <0.00%> (ø)`
pyresample/test/test_spherical.py	`100.00% <0.00%> (ø)`
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

coveralls · 2022-06-26T20:00:09Z

Coverage increased (+0.3%) to 94.063% when pulling 79edf86 on djhoese:feature-antimeridian-mode into 0cb8914 on pytroll:main.

pnuu

Couple of inline comments. In general I guess this LGTM, but there should be some documentation at least listing the options.

pnuu · 2022-06-27T16:16:43Z

pyresample/geometry.py

+            else:
+                xmin = np.nanmin(xarr[xarr >= 0])
+                xmax = np.nanmax(xarr[xarr < 0]) + 360
+            if antimeridian_mode == "modify_projection":


Would shift_prime_median be more specific?

Hm interesting idea. I think I chose "projection" in case other projections in the future may need different types of modifications. For example, if we were trying to support someone using +x and +y parameters in their projection definition, I'm not sure the +pm would be the best thing to modify.

pnuu · 2022-06-27T16:17:36Z

pyresample/geometry.py

-            xmin = np.nanmin(xarr[xarr >= 0])
-            xmax = np.nanmax(xarr[xarr < 0]) + 360
-        return xmin, ymin, xmax, ymax
+            if antimeridian_mode == "global_extents":


This is the only option that doesn't have an imperative mode, so should it be use_global_extents?

Good point. We could also remove the verb and just do global_extents, nearest_extents, and projection...eh I'm not sure I like that. And yeah I need to document these. I thought I had but obviously not.

gerritholl

Just a few clarification questions on the documentation. Looks good.

pyresample/geometry.py

djhoese · 2022-06-30T13:59:59Z

pyresample/geometry.py

+        antimeridian_mode:
+            How to handle lon/lat data crossing the anti-meridian of the
+            projection. This currently only effects lon/lat geographic
+            projections and data cases not covering the north or south pole.
+            The possible options are:
+
+            * modify_extents: Set the X bounds to the edges of the data, but
+                add 360 to the right-most bound. This has the effect of making
+                the area coordinates continuous from the left side to the
+                right side. However, this means that some coordinates will be
+                outside the coordinate space of the projection. Although most
+                PROJ and pyresample functionality can handle this there may be
+                some edge cases.
+            * modify_projection: Change the prime meridian of the projection
+                from 0 degrees longitude to 180 degrees longitude. This has
+                the effect of putting the data on a continuous coordinate
+                system. However, this means that comparing data resampled to
+                this resulting area and an area not over the anti-meridian
+                would be more difficult.
+            * global_extents: Ignore the bounds of the data and use -180/180
+                degrees as the west and east bounds of the data. This will
+                generate a large output area, but with the benefit of keeping
+                the data on the original projection. Note that some resampling
+                methods may produce artifacts when resampling on the edge of
+                the area (the anti-meridian).


@mraspaud @pnuu @gerritholl I think the last remaining question is what are the best names for these parameters. In another comment Panu pointed out that global_extents is the only one that doesn't have an imperative verb at the front. He also suggested using "shift_prime_meridian" instead of "modify_projection", but my worry with that is that in the future modifying the projection to support the data may not only include shifting the prime meridian.

Thoughts? Opinions?

make_extent_global or globalize_extent would be an imperative form of global_extents. But it's not actually global, is it? It covers all longitudes, but must not necessarily cover all latitudes. It anyway should affect only the longitude extent, not the latitude extent, shouldn't it? The docstring in lines 1091 and 1092 mentions lon/lat, but this should be only about lon?

About modify_projection: it would seem that it's not accurate to speak of a projection at all here. When we describe data in lat/lon then the data are unprojected, or are they?

I would replace projection by CRS and change modify_projection to modify_CRS. I can see how a future case of, for example, data covering a pole may modify lat/lons by putting the pole somewhere different, yet retain spherical coordinates therefore still not being technically a projection (but rather a confusion ☺).

Good point bringing up the poles. I should update the docs to reference that more. The extent handling is ignored if the data covers a pole as there isn't a "perfect" way to make the data contiguous on a single small area.

The poles are a coordinate singularity, so lat/lon coordinates are simply not suitable around the poles.

I like modify_crs (lower case, to be more pythonic). And indeed, I was only thinking about the current case of adjusting the prime meridian.

Similarly global_extents would be future proof for any projection type, although what @gerritholl sait about the actual globality is a valid point. Some (hasty!) suggestions for names for this: use_full_width(_extents), wrap_x_extents, cover_all_meridians.

The thing about the "global_extents" not being global is a little odd. Right now it is global in the longitudinal dimension, but not necessarily in latitude. I would hope users wouldn't be too confused that a parameter dealing with the anti-meridian would only effect meridians and not parallels. We could call it expand_extents which can give the general impression of "we're going to increase the range of the extents to fit more than just the data".

ignore_extents instead of global_extents? and modify_crs sounds good.

Why "ignore"? What is being ignored? Oh you mean ignoring the extents of the data...eh I'm not sure I like that. I'll change to modify_crs for that one, but still not sure on extents.

pyresample/geometry.py

mraspaud

The logic seems sound, great work! I'd just like to get rid of some code duplication

pyresample/geometry.py

mraspaud

LGTM, thanks for the hard work!

pnuu

LGTM

djhoese added 2 commits May 5, 2022 21:01

Fix create_area_def tests being in the wrong test class

6025c54

Add initial tests and proof of concept for antimeridian_mode

353d621

djhoese added the enhancement label May 6, 2022

djhoese self-assigned this May 6, 2022

djhoese commented May 6, 2022

View reviewed changes

pyresample/geometry.py Outdated Show resolved Hide resolved

djhoese added 2 commits June 26, 2022 14:36

Refactor DynamicAreaDefinition to handle projection bounds for corners

db198b7

Add future annotations import

f2d5e8b

djhoese changed the title ~~[WIP] Add 'antimeridian_mode' to DynamicAreaDefinition~~ Add 'antimeridian_mode' to DynamicAreaDefinition Jun 27, 2022

djhoese marked this pull request as ready for review June 27, 2022 15:46

pnuu requested changes Jun 27, 2022

View reviewed changes

gerritholl approved these changes Jun 28, 2022

View reviewed changes

pyresample/geometry.py Outdated Show resolved Hide resolved

pyresample/geometry.py Outdated Show resolved Hide resolved

Add documentation for antimeridian_mode

88ae028

djhoese commented Jun 30, 2022

View reviewed changes

gerritholl reviewed Jun 30, 2022

View reviewed changes

pyresample/geometry.py Show resolved Hide resolved

gerritholl reviewed Jun 30, 2022

View reviewed changes

pyresample/geometry.py Outdated Show resolved Hide resolved

mraspaud requested changes Jul 27, 2022

View reviewed changes

pyresample/geometry.py Show resolved Hide resolved

pyresample/geometry.py Outdated Show resolved Hide resolved

pyresample/geometry.py Outdated Show resolved Hide resolved

pyresample/geometry.py Outdated Show resolved Hide resolved

djhoese added 3 commits July 27, 2022 09:15

Cleanup antimeridian docstring and code

d935fb2

Refactor antimeridian handling in DynamicAreaDefinition

a7cbc1c

Rename modify_projection antimeridian mode to modify_crs

faa9da6

mraspaud reviewed Jul 28, 2022

View reviewed changes

pyresample/geometry.py Outdated Show resolved Hide resolved

Refactor antimeridian wrapping to be clearer

79edf86

mraspaud approved these changes Jul 28, 2022

View reviewed changes

pnuu approved these changes Jul 28, 2022

View reviewed changes

djhoese merged commit 6664669 into pytroll:main Jul 28, 2022

djhoese deleted the feature-antimeridian-mode branch July 28, 2022 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'antimeridian_mode' to DynamicAreaDefinition #431

Add 'antimeridian_mode' to DynamicAreaDefinition #431

djhoese commented May 6, 2022

codecov bot commented Jun 26, 2022 •

edited

Loading

coveralls commented Jun 26, 2022 •

edited

Loading

pnuu left a comment

pnuu Jun 27, 2022

djhoese Jun 27, 2022

pnuu Jun 27, 2022

djhoese Jun 27, 2022

gerritholl left a comment

djhoese Jun 30, 2022

gerritholl Jun 30, 2022

gerritholl Jun 30, 2022

djhoese Jun 30, 2022

gerritholl Jun 30, 2022

pnuu Jun 30, 2022

djhoese Jul 1, 2022

mraspaud Jul 27, 2022

djhoese Jul 27, 2022

mraspaud left a comment

mraspaud left a comment

pnuu left a comment

Add 'antimeridian_mode' to DynamicAreaDefinition #431

Add 'antimeridian_mode' to DynamicAreaDefinition #431

Conversation

djhoese commented May 6, 2022

codecov bot commented Jun 26, 2022 • edited Loading

Codecov Report

coveralls commented Jun 26, 2022 • edited Loading

pnuu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerritholl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud left a comment

Choose a reason for hiding this comment

pnuu left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 26, 2022 •

edited

Loading

coveralls commented Jun 26, 2022 •

edited

Loading