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

Combine wrapped collection into GeoMesh data when get_array is called #1656

Merged
merged 3 commits into from
Oct 11, 2020

Conversation

sdeastham
Copy link
Contributor

When calling get_array with a QuadMesh object (from pcolormesh), the returned array only included the unmasked data. This change fills in the masked data when present. This follows from the discussion in #1654.

Rationale

The pcolormesh set_array method now correctly handles the wrapped, masked collections which can be generated when polygons cross the antimeridian. However, the get_array method still only returns the data for the standard cells (those which did not cross the antimeridian). This caused unexpected behavior when running, for example:

im = ax.pcolormesh(...)
im.set_array(im.get_array()*1.0)

This would result in the same plot being shown, but now with the cells at 180E missing. This fix modifies the get_array call to compensate.

Implications

Calling get_array now returns the complete array, with the masked elements filled in.

When calling get_array with a QuadMesh object (from pcolormesh), the
returned array only includes the unmasked data. This change fills in
the masked data when present.
@greglucas
Copy link
Contributor

Looks like a good way to do it to me, @sdeastham!

Can you add a test for this too? As simple as just putting mesh.get_array() == Z_input for a wrapped and not-wrapped case.

Also, it looks like you've got one failing test currently. That is actually one that I just put in to make sure the cells were being wrapped properly. A good fix for that test would be to assert that the _wrapped_collection attribute is present in that case (which implies it was wrapped).

@greglucas
Copy link
Contributor

ping @sdeastham. I think all that is needed is updated tests here.

@sdeastham
Copy link
Contributor Author

sdeastham commented Oct 10, 2020

@greglucas Got it. Sorry about the delay - I've never used pytest before and am having terrible trouble getting it to do anything except throw ModuleNotFoundError: No module named 'cartopy._version'...

@greglucas
Copy link
Contributor

No worries at all. I think that error can be fixed by adding the pyargs argument to pytest: pytest --pyargs cartopy. See #1371 for a few different ways to run the tests. It depends on how you installed your local version as well, so if you installed an editable version with pip install -e . or just a standard installation pip install .

A new test has been added which verifies that the data returned by
a pcolormesh get_array() command matches the data which was first
given to the pcolormesh command, even if wrapping has occurred.
One of the other tests (pcolormesh_diagonal_wrap) was also failing
due to a faulty check, which has been removed.
lib/cartopy/tests/mpl/test_mpl_integration.py Outdated Show resolved Hide resolved
@@ -321,6 +321,48 @@ def test_pcolormesh_global_with_wrap1():
ax.coastlines()
ax.set_global() # make sure everything is visible

#@pytest.mark.natural_earth
#@ImageTesting(['pcolormesh_global_wrap1'], tolerance=1.27)

Choose a reason for hiding this comment

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

E265 block comment should start with '# '

@@ -321,6 +321,48 @@ def test_pcolormesh_global_with_wrap1():
ax.coastlines()
ax.set_global() # make sure everything is visible

#@pytest.mark.natural_earth
#@ImageTesting(['pcolormesh_global_wrap1'], tolerance=1.27)
def test_pcolormesh_get_array_with_mask():

Choose a reason for hiding this comment

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

E302 expected 2 blank lines, found 1

assert c._wrapped_collection_fix is not None, \
'No pcolormesh wrapping was done when it should have been.'

assert np.array_equal(data.ravel(),c.get_array()), \

Choose a reason for hiding this comment

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

E231 missing whitespace after ','

ax.coastlines()
ax.set_global() # make sure everything is visible

assert getattr(c,"_wrapped_collection_fix",None) is None, \

Choose a reason for hiding this comment

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

E231 missing whitespace after ','

assert getattr(c,"_wrapped_collection_fix",None) is None, \
'pcolormesh wrapping was done when it should not have been.'

assert np.array_equal(data2.ravel(),c.get_array()), \

Choose a reason for hiding this comment

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

E231 missing whitespace after ','

Stickler Bot identified six issues with the previous commit. Two
were with commented-out code which was no longer necessary, one was
because of a missing newline, and three were because of missing
whitespace after commas. All have now been resolved.
@sdeastham
Copy link
Contributor Author

@greglucas Thanks again for your patience! I've attempted to build some tests which test that get_array actually returns the expected data (albeit 1-D), as well as modifying the diagonal_wrap test to prevent the failure you mentioned. I wasn't 100% certain on what you intended with that one, so please do let me know if you'd like anything changed (and indeed if you see changes needed with anything else in the new tests).

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

@sdeastham, glad you stuck with it and got all the tests working. This looks good to me! Thanks for contributing and hope to see you back again!

@greglucas greglucas merged commit d12c86c into SciTools:master Oct 11, 2020
@QuLogic QuLogic added this to the 0.19 milestone Oct 13, 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.

4 participants