-
Notifications
You must be signed in to change notification settings - Fork 391
Implemented IGH Oceanic View #1561
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
Conversation
lib/cartopy/crs.py
Outdated
| """ | ||
| input_point_nans = np.isnan(x) | np.isnan(y) | ||
| input_point_nans = np.isnan(x) | np.isnan(y) | \ | ||
| np.isinf(x) | np.isinf(y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E127 continuation line over-indented for visual indent
|
Now that Proj 7.1.0 is out, we can add this. However, it will need a check that the version of Proj is new enough, like |
|
OK - sounds good. What is the timeline for the next version release?
|
|
No specific plans, but probably not for a couple months. |
|
@QuLogic - I added in a PROJ version check and updated the documentation. I didn't see a |
|
Can you rebase to get working CI, and ensure that tests are passing? |
|
I rebased against the current |
|
@QuLogic - I don't really understand the tests well enough to understand how this is failing. Are you able to help me? |
greglucas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is really close. The minimum version test is currently failing due to an old proj being used. You'll need to add a pytest.skipif to check versions for this, something similar to this:
cartopy/lib/cartopy/tests/mpl/test_mpl_integration.py
Lines 243 to 244 in 4ca93c1
| @pytest.mark.skipif(ccrs.PROJ4_VERSION < (5, 2, 0), | |
| reason='Proj is too old.') |
But probably within the parameterized function itself rather than as a decorator.
Do you want to add an image comparison for the new projection?
docs/source/whats_new.rst
Outdated
| Features | ||
| -------- | ||
|
|
||
| * John Krasting added the ability to use the oceanic view |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for the previous release What's New. We generally make this while doing the release. You can add yourself to the contributors list now though.
https://github.com/SciTools/cartopy/blob/master/docs/source/contributors.rst
lib/cartopy/crs.py
Outdated
| """ | ||
| Parameters | ||
| ---------- | ||
| central_longitude: optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| central_longitude: optional | |
| central_longitude : float, optional |
All of these should have a space after the name, then the type, followed by optional. See here for the parameter spec: https://numpydoc.readthedocs.io/en/latest/format.html#parameters
|
Thanks for your help @greglucas! In regard to this suggestion:
Did you mean add a figure in |
|
Yes, I think that would be a good place for it. Just to make sure the image for the ocean view is coming out as you expect. |
5416edd to
1bc6542
Compare
|
@greglucas - thanks for the feedback. I think everything is addressed now. This PR is blocked again from running the CI, but everything seems to check out OK on my fork: https://github.com/jkrasting/cartopy/actions/runs/929096625 |
greglucas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a nice addition and good to go. I'll hold off merging for a few days in case anyone else has comments/suggestions on it.
Thanks for your help @greglucas. Looking forward to seeing this in a future release! |
|
Can you rebase to fix the conflicts? |
- The ocean emphasis for the Interrupted Goode Homolosine projection (imh_o) requires proj version 7.1.0 or later. - Option to select 'land' or 'ocean' emphasis - Increaed n resolution from 31 points to 91 points - Added docstring for class - Added images of both land and ocean views
- Applies to InterruptedGoodeHomolosine views
- Added tests for ocean view in addition to the existing land view - Values should be the same for both views
- While the emphasis="land" option is the default, these modifications will ensure that the MPL tests are always run with the option.
- The Interrupted Goode Homolosine ocean projection requires Proj version 7.1.0 or greater.
- Added tests for both land and ocean
- Follows NumPy formatting
|
Yes, just rebased now. |
|
@greglucas, @QuLogic - looks like this needs a workflow approval for parts of the CI to run. |
|
Thanks @jkrasting! |

a new projection (imh_o)
IGH Default View:
IGH New Optional Ocean View:
Rationale
This update provides new functionality for oceanographic plots that was not previously possible with the land-only focus of the current IGH projection.
Implications
None.