Add a function to join/enclose areas. #306

gerritholl · 2020-10-22T14:31:28Z

Add a function to calculate an area enclosing one or more other areas.

Closes Add a function to join/enclose areas. #306
Tests added
Tests passed
Passes git diff origin/master **/*py | flake8 --diff
Fully documented

Add a function to calculate an area enclosing one or more other areas.

ghost · 2020-10-22T14:31:52Z

Congratulations 🎉. DeepCode analyzed your code in 2.783 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

I disagree with deepcodes suggestion. Explicit is better than implicit. PEP 8 does NOT recommend against ``if len(x) == 0``, it recommends against ``if not len(x)``. According to "Explicit is better than implicit", ``if len(x) == 0`` should be preferable and clearer. However, I will adapt it anyway as deepcode is the boss here?

gerritholl · 2020-10-22T14:40:20Z

The AppVeyor build failure appears unrelated to this PR.

djhoese · 2020-10-22T14:52:06Z

This reminds me of gdal_merge.py, maybe merge is another word for this functionality?

djhoese · 2020-10-22T14:53:17Z

Also what are your thoughts on this being a method on the AreaDefinition itself? We could even overload the __add__ method so you could do area1 + area2 + area3 and get the enclosing area.

gerritholl · 2020-10-22T15:15:17Z

Merge could be another word. I chose enclose because the resulting area includes not only the area covered by the input areas, but also the area in-between. I borrowed this word from the portion library.

I'm open to add it as a classmethod/constructor on the AreaDefinition, but I'm not sure about overloading + because I'm not sure the rules usually associated with addition necessarily hold here. For example, for regular addition, if a + b + c == a + c, then a + b == a. This does not hold for merging or enclosing areas.

djhoese · 2020-10-22T15:21:34Z

Good points. Maybe |, but regardless this is "extra" and not something that needs to be done.

gerritholl · 2020-10-22T16:10:24Z

| is like joining sets, which also doesn't really hold, because A | B in sets should only include what is in either A or B, but an enclosure may include areas in neither A nor B. I would reserve those operators for other operations which might be more appropriate in the future (for example, returning a StackedAreaDefinition).

gerritholl · 2020-10-22T16:14:34Z

If Python had range literals that would be an appropriate syntax for this. Wild idea: create a special object enc (or named otherwise) which overloads slice syntax: enc[A:B] producing an AreaDefinition that is the enclosure of A and B. ☺

djhoese · 2020-10-22T16:21:45Z

Alright, fine. I suppose the | doesn't completely line up with set operations. I was more thinking the "opposite" of an intersection (&) where the result would be only the overlapping portions of the image.

I wonder if we can or should come up with notation for these types of operations for a pyresample 2.0 kind of interface.

mraspaud · 2020-10-23T06:25:10Z

How about union vs intersection for clarity?

gerritholl · 2020-10-23T07:23:28Z

@mraspaud The enclosure is neither a union nor an intersection. It's the geo-2d equivalent of what pyinterval calls the enclosure. The union {2, 4} | {6, 8} does not include 5, but if we have the intervals (2, 4] and (6, 8], then their span (2, 8] does include 5. This is the 2-dimensional equivalent of such a span/range/enclosure.

Any actual union or intersection function would either need to be very restrictive on its inputs (such as combine_area_extents_vertical), or (allow to) return a StackedAreaDefinition, or we would need to allow for non-rectangular and even non-contiguous areas. The latter option would seem quite involved.

codecov · 2020-10-28T07:41:51Z

Codecov Report

Merging #306 (bdb6ccb) into master (3869ce8) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #306      +/-   ##
==========================================
+ Coverage   92.97%   92.99%   +0.02%     
==========================================
  Files          48       48              
  Lines        9764     9798      +34     
==========================================
+ Hits         9078     9112      +34     
  Misses        686      686

Flag	Coverage Δ
unittests	`92.99% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
pyresample/geometry.py	`85.32% <100.00%> (+0.22%)`	⬆️
pyresample/test/test_geometry.py	`98.49% <100.00%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3869ce8...bdb6ccb. Read the comment docs.

coveralls · 2020-10-28T07:53:31Z

Coverage increased (+0.006%) to 92.66% when pulling 25542c5 on gerritholl:area-join-utility into bbd8054 on pytroll:master.

coveralls · 2020-10-28T07:53:31Z

Coverage increased (+0.02%) to 92.988% when pulling bdb6ccb on gerritholl:area-join-utility into 3869ce8 on pytroll:master.

pnuu

LGTM.

djhoese

Small request to use CRS. Otherwise for the name, I found this https://en.wikipedia.org/wiki/Minimum_bounding_box

So enclosing is definitely the right concept. However, should a function name use a verb? Or indicate an action? Something like "enclose_areas" or "enclosed_area" or "enclosing_area_box" (nah maybe not this one). I suppose "area_enclosure" works, I'm just worried I'm not going to find it later. Should something be added to the documentation? I'm not sure where given the current (terrible) organization of the sphinx docs.

djhoese · 2021-03-09T12:39:46Z

pyresample/geometry.py

+
+ return create_area_def(
+ area_id=area_id,
+ projection=first.proj_dict,


After #332, this should be changed to .crs. Actually, it could be done now and should still work. Passing PROJ dicts in is fine, using them after is not. Ah you also use proj_dict below to get the units. This shouldn't be necessary as create_area_def should determine the units from the CRS information.

If I change first.proj_dict to first.crs the test fails with TypeError: Wrong type for projection: <class 'pyproj.crs.crs.CRS'>. Expected dict or string. in _get_proj_data, which verifies that the projection is either a dict or a string.

🤔 I think in #332 that should work. Once some of my more recent PRs are merged maybe we can test this idea again.

Should I wait for merger of #332 and then adapt the code here?

Merged just now. If you have the time to merge master into this PR and test some things that'd be great. Thanks.

Replaced proj_dict by crs.

Changed name to enclose_areas.

Change the name of the function area_enclosure to enclose_areas.

Remove duplicate "import pytest" in test_geometry.py.

mraspaud

LGTM, nice feature!

Add a function to join/enclose areas.

4f6524b

Add a function to calculate an area enclosing one or more other areas.

Merge branch 'master' into area-join-utility

25542c5

gerritholl requested review from mraspaud and pnuu October 30, 2020 16:30

pnuu approved these changes Mar 9, 2021

View reviewed changes

djhoese requested changes Mar 9, 2021

View reviewed changes

gerritholl added 3 commits March 9, 2021 13:55

Merge branch 'master' into area-join-utility

8cc3721

Merge branch 'master' into area-join-utility

6699f9d

Merge branch 'master' into area-join-utility

3bcc187

mraspaud assigned gerritholl Mar 12, 2021

mraspaud added the enhancement label Mar 12, 2021

gerritholl added 3 commits March 12, 2021 14:10

Merge branch 'master' into area-join-utility

50bafda

Replace proj_dict by crs

f1e048f

Rename area_enclosure to enclose_areas

599fc22

Change the name of the function area_enclosure to enclose_areas.

gerritholl requested a review from djhoese March 12, 2021 13:39

Remove duplicate import

bdb6ccb

Remove duplicate "import pytest" in test_geometry.py.

mraspaud approved these changes Mar 12, 2021

View reviewed changes

mraspaud merged commit 6d2e8bb into pytroll:master Mar 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a function to join/enclose areas. #306

Add a function to join/enclose areas. #306

gerritholl commented Oct 22, 2020

ghost commented Oct 22, 2020 •

edited by ghost

Loading

gerritholl commented Oct 22, 2020 •

edited

Loading

djhoese commented Oct 22, 2020

djhoese commented Oct 22, 2020

gerritholl commented Oct 22, 2020

djhoese commented Oct 22, 2020

gerritholl commented Oct 22, 2020

gerritholl commented Oct 22, 2020

djhoese commented Oct 22, 2020

mraspaud commented Oct 23, 2020

gerritholl commented Oct 23, 2020

codecov bot commented Oct 28, 2020 •

edited

Loading

coveralls commented Oct 28, 2020

coveralls commented Oct 28, 2020 •

edited

Loading

pnuu left a comment

djhoese left a comment

djhoese Mar 9, 2021

gerritholl Mar 9, 2021

djhoese Mar 9, 2021

gerritholl Mar 10, 2021 •

edited

Loading

djhoese Mar 10, 2021

gerritholl Mar 12, 2021

gerritholl Mar 12, 2021

mraspaud left a comment

Add a function to join/enclose areas. #306

Add a function to join/enclose areas. #306

Conversation

gerritholl commented Oct 22, 2020

ghost commented Oct 22, 2020 • edited by ghost Loading

👉 View analysis in DeepCode’s Dashboard | Configure the bot

gerritholl commented Oct 22, 2020 • edited Loading

djhoese commented Oct 22, 2020

djhoese commented Oct 22, 2020

gerritholl commented Oct 22, 2020

djhoese commented Oct 22, 2020

gerritholl commented Oct 22, 2020

gerritholl commented Oct 22, 2020

djhoese commented Oct 22, 2020

mraspaud commented Oct 23, 2020

gerritholl commented Oct 23, 2020

codecov bot commented Oct 28, 2020 • edited Loading

Codecov Report

coveralls commented Oct 28, 2020

coveralls commented Oct 28, 2020 • edited Loading

pnuu left a comment

Choose a reason for hiding this comment

djhoese left a comment

Choose a reason for hiding this comment

djhoese Mar 9, 2021

Choose a reason for hiding this comment

gerritholl Mar 9, 2021

Choose a reason for hiding this comment

djhoese Mar 9, 2021

Choose a reason for hiding this comment

gerritholl Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

djhoese Mar 10, 2021

Choose a reason for hiding this comment

gerritholl Mar 12, 2021

Choose a reason for hiding this comment

gerritholl Mar 12, 2021

Choose a reason for hiding this comment

mraspaud left a comment

Choose a reason for hiding this comment

ghost commented Oct 22, 2020 •

edited by ghost

Loading

gerritholl commented Oct 22, 2020 •

edited

Loading

codecov bot commented Oct 28, 2020 •

edited

Loading

coveralls commented Oct 28, 2020 •

edited

Loading

gerritholl Mar 10, 2021 •

edited

Loading