Skip to content
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

Implemented IGH Oceanic View #2226

Merged
merged 24 commits into from May 19, 2020
Merged

Implemented IGH Oceanic View #2226

merged 24 commits into from May 19, 2020

Conversation

jkrasting
Copy link
Contributor

@jkrasting jkrasting commented May 15, 2020

  • The current implementation of the Interrupted Goode Homolosine
    projection emphasizes land area. This is a compliment projection
    that emphasizes ocean area.
  • A value of lon0=-160 produces a reasonable real-world map.
  • Tested update using cartopy:

imh_ocean_view

- The current implementation of the Interrupted Goode Homolosine
  projection emphasizes land area. This is a compliment projection
  that emphasizes ocean area.
- A value of lon0=-150 produces a reasonable real-world map.
Copy link
Member

@kbevers kbevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this. Very cool to get this projection in PROJ!

There's a few things missing from this PR:

  1. igh_o.png does not seem to be commited
  2. Tests - can be added in test\gie\, either as a standalone file or in e.g. builtins.gie.
  3. Less important, but still relevant: More descriptive docs. It would be nice to have a short introduction that explains what this projection is.

src/projections/igh_o.cpp Outdated Show resolved Hide resolved
src/projections/igh_o.cpp Outdated Show resolved Hide resolved
src/projections/igh_o.cpp Outdated Show resolved Hide resolved
src/projections/igh_o.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@jkrasting jkrasting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback - just submitted a PR with a test case and image.

@jkrasting
Copy link
Contributor Author

FYI -- see complement pull request with cartopy

@rouault
Copy link
Member

rouault commented May 15, 2020

/Users/travis/build/OSGeo/PROJ/docs/source/operations/projections/igh_o.rst:document isn't included in any toctree

==> It should be referenced in docs/source/operations/projections/index.rst

- new name is more descriptive
- replaced #define with const declaration
- In igh_o.cpp:  bool ok = false
- Applies to igh and igh_o projections
src/projections/igh.cpp Outdated Show resolved Hide resolved
@jkrasting jkrasting requested a review from kbevers May 15, 2020 23:43
@rouault
Copy link
Member

rouault commented May 15, 2020

I actually see this projection was a feature request of #1442 . But in #1442 (comment) , the interruptions values suggested seem different to the one you propose, but the projection maps look similar. I'm not sure who is right or who is wrong or maybe if both proposals are identical, but wanted to raise this and if we could know if there's interoperability with the equivalent ArcGIS projection (https://desktop.arcgis.com/en/arcmap/10.3/guide-books/map-projections/goodes-homolosine.htm)

@jkrasting
Copy link
Contributor Author

Yes I see some differences. Thanks for pointing this out. The original reference from 1925 is here: https://www.tandfonline.com/doi/abs/10.1080/00045602509356949

Some adjustments are needed, but this version is pretty close

Copy link
Member

@kbevers kbevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the original paper doesn't mention how the zones are divided in the oceanic view, so there's no help there. What's implemented now is likely good enough and I'm sure we'll made aware if it isn't.

I think this is looking good now. Thanks for putting in the extra effort improving the code, very much appreaciated.

@jkrasting
Copy link
Contributor Author

No, but the map is instructive. It looks like lon_0 should be -160 and some minor adjustments to the true longitudes and split lines are needed. Will update and push a new commit today. Thats @kbevers and @rouault for pointing this out.

@rouault
Copy link
Member

rouault commented May 16, 2020

Another source for inspiration / comparison is this implementation based on d3.js: https://bl.ocks.org/Fil/d90a94d7333bfef753e92a4abb7611b4 / https://observablehq.com/@d3/interrupted-homolosine-oceans. The convention for the lobes is explained at https://github.com/d3/d3-geo-projection/#interrupted-projections . Looking at the values for the standard IGH, they seem to match with PROJ igh (at list the limits of zones)

- Updates based on Goode, JP, 1925. (DOI: 10.1080/00045602509356949)
- High-latitudes "lobes" added for projections #1 and OSGeo#3
- Central longitude changed to -160
- Added fill reference to docstrings for igh.cpp and igh_o.cpp
@jkrasting
Copy link
Contributor Author

jkrasting commented May 16, 2020

The lobes are present in the original reference. Zones 1 and 3 have them for the ocean projection. I added them into the latest commit.

image

docs/plot/plotdefs.json Outdated Show resolved Hide resolved
test/gie/builtins.gie Show resolved Hide resolved
- Each of the 12 sub-projections is tested independently.
@@ -2183,10 +2209,35 @@ expect -0.001790497 0.000895247
accept -200 -100
expect -0.001790496 -0.000895247

accept -100.0 22.0
Copy link
Member

@rouault rouault May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those points in the inverse direction are all close to lat=lon=0 and not super relevant. I would just remove them completely, but as suggested previously, after each expect statement above add a "roundtrip 1" line, that will check that when using the inverse path the values of expect are back projected to the ones of accept

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added in the roundtrip option for all zones in my latest commit, but I am seeing failures with infs in zones 11 and 12. I am stumped. Do you see anything obvious with how I implemented the tests?

test/gie/builtins.gie Outdated Show resolved Hide resolved
Copy link
Member

@rouault rouault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests can be simplified & improved. See 2 comments

- Tests fail for zones 11 & 12 with inf results
- Zones 11 & 12 were reversed; error caused roundtrip test
  to fail
@jkrasting
Copy link
Contributor Author

0 was fine. z is a int. assigning it a boolean value is confusing (even if false will ultimately evaluate to 0 once cast to int)

I reverted the commit.

@jkrasting jkrasting requested a review from rouault May 18, 2020 23:15
@rouault
Copy link
Member

rouault commented May 18, 2020

did you regenerate the image after your latest adjustments ?

@jkrasting
Copy link
Contributor Author

did you regenerate the image after your latest adjustments ?

It shouldn't change the image but I'll double check to confirm.

@rouault
Copy link
Member

rouault commented May 18, 2020

I would expect bf6a8dc to have impacts on the forward path, no ?

@jkrasting
Copy link
Contributor Author

jkrasting commented May 18, 2020

The plot is identical. The change in bf6a8dc is aesthetic and was done to be consistent to match igh.cpp. That block of code identifies the delta value for latitude which did not change when comparing those two zones.

@rouault rouault merged commit 2e54703 into OSGeo:master May 19, 2020
2 checks passed
@rouault rouault added this to the 7.1.0 milestone May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants