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

improvement of basemap.addcyclic #138

Merged
merged 5 commits into from Feb 22, 2014
Merged

improvement of basemap.addcyclic #138

merged 5 commits into from Feb 22, 2014

Conversation

j08lue
Copy link
Contributor

@j08lue j08lue commented Feb 17, 2014

In its current implementation, basemap.addcyclic actually does not work for 2D arrays and fixing the number of input arrays to 2 is also somewhat arbitrary.

The proposed version works for any number of n-dimensional arrays in arr and has the option to specify the axis along which to add the points.

The new function is not backward compatible. However, users will merely have to add a pair of parentheses around their input arguments, as described in the docstring.

@jswhit
Copy link
Contributor

jswhit commented Feb 17, 2014

Thanks! Would it be that hard to make it backward compatible? I really would rather not break existing code.

@jswhit
Copy link
Contributor

jswhit commented Feb 17, 2014

One solution would be to create a new function (addcyclic2), and then add a deprecation warning in addcyclic. After the next release, addcyclic2 could replace addcyclic.

@j08lue
Copy link
Contributor Author

j08lue commented Feb 18, 2014

Thanks for your comment, Jeff. I made the function backward-compatible. It is actually more elegant this way than in the previous version.

@jswhit
Copy link
Contributor

jswhit commented Feb 19, 2014

Great - that works. Please add a Changelog entry, and add yourself to the list of contributors in README.md, and I'll merge.

@j08lue
Copy link
Contributor Author

j08lue commented Feb 20, 2014

Great, Jeff, I did as you told. I do not know where my name belongs in the list with respect to my small contribution... Please feel free to rearrange.

jswhit added a commit that referenced this pull request Feb 22, 2014
 improvement of basemap.addcyclic
@jswhit jswhit merged commit 4b459b0 into matplotlib:master Feb 22, 2014
@jswhit
Copy link
Contributor

jswhit commented Feb 22, 2014

Thanks a lot Jonas - merging now...

@jswhit
Copy link
Contributor

jswhit commented Mar 7, 2014

I'm going to revert this commit in light of issue #139. j08blue, when you get a chance can you revisit this and see if there is a way to preserve the old behaviour (which treats the longitudes array differently) with your more elegant implementation?

jswhit added a commit that referenced this pull request Mar 7, 2014
139.

This reverts commit 4b459b0, reversing
changes made to a68aeb3.
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

2 participants