Skip to content

Shiftdata fix #332

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

Merged
merged 14 commits into from
Jan 4, 2017
Merged

Shiftdata fix #332

merged 14 commits into from
Jan 4, 2017

Conversation

guziy
Copy link
Contributor

@guziy guziy commented Jan 2, 2017

I've tried to avoid the portion of shiftdata trying to do the wraparound fix for the scatter. For this I've added a keyword argument to the shiftdata and then execute the wraparound fix only if the argument is True. It is True by default but is set to Falsewhen the plotfunc is scatter.

That should close the #197, but to be sure I would like to wait for the failing example from @kiwi937.

Refactor: move masking of the output longitudes and data arrays for 1d and 2d cases to a single place, since they are the same.

Cheers

@WeatherGod
Copy link
Member

I'd feel better about this if there were some unit tests added. Also, what was the reason for excluding scatter() from shiftdata? Perhaps it would be good to add some more explanation into docstrings and/or comments.

Also, and perhaps this is because the docstring for shiftdata isn't all that great, but an initial read of the doc for the bool flag kind of makes it seem like if it is false, then the function is a no-op, which isn't the case.

@guziy
Copy link
Contributor Author

guziy commented Jan 3, 2017

Thanks for your comments @WeatherGod:

I'd feel better about this if there were some unit tests added

I understand. I'll add few tests.

what was the reason for excluding scatter() from shiftdata?

scatter is not completely excluded from the shiftdata, but the reindexing (the reindexing which is supposed to set lower indices for the points with coordinates to the right of lon_0 + 180) is not performed in the case of scatter. This reindexing is important when a field is plotted and is based on the fact that the neighbouring points are close, which is not necessarily true for scatter. With the modification, the only thing done in case of scatter is converting longitudes to the (lon_0 -180, lon_0 + 180) range, so that it is still consistent with pcolormesh or contourf results..

Also, and perhaps this is because the docstring for shiftdata isn't all that great, but an initial read of the doc for the bool flag kind of makes it seem like if it is false, then the function is a no-op, which isn't the case.

I'll try to make it clearer.

Cheers

@WeatherGod WeatherGod merged commit 845fb6d into matplotlib:master Jan 4, 2017
@WeatherGod
Copy link
Member

Awesome work!

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.

2 participants