Skip to content

Merge 1.5.x to 2.x #6443

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
May 21, 2016
Merged

Merge 1.5.x to 2.x #6443

merged 13 commits into from
May 21, 2016

Conversation

jenshnielsen
Copy link
Member

There was a conflict in axes3d.py so letting Travis verify that this works. @WeatherGod Can you have a look at that change?

WeatherGod and others added 7 commits May 17, 2016 09:58
@WeatherGod
Defer to 2D scatter() for handling of 'c'. Closes matplotlib#5974.
8e2b7a9
@mdboom
Merge pull request matplotlib#6439 from WeatherGod/scatter3d_color
fc654e6 
Defer to 2D scatter() for handling of 'c'. Closes matplotlib#5974.
@dopplershift
MNT: Raise an error for invalid args in animation.save.
7d1f717 
When passed arguments that are only used when creating a new MovieWriter
instance, raise an error if writer is an existing instance rather than
silently ignoring the arguments. Also document the fact that these
arguments are invalid in that case.
@dopplershift
MNT: Improve error message when saving animation fails.
ecfc1e7
@dopplershift
MNT: Add original exception to raised IOError.
5e5999f
@dopplershift
MNT: Re-arrange docs to avoid repeating save verbiage.
7c98a98
@WeatherGod
Merge pull request matplotlib#6441 from dopplershift/anim-errors
e2549cd 
Animation error handling (Fixes matplotlib#6416)
@jenshnielsen jenshnielsen added this to the 2.0 (style change major release) milestone May 18, 2016
WeatherGod
WeatherGod reviewed May 18, 2016
View reviewed changes
lib/mpl_toolkits/mplot3d/axes3d.py
@@ -2271,11 +2273,12 @@ def scatter(self, xs, ys, zs=0, zdir='z', s=20, c=None, depthshade=True,

if c is None:
c = self._get_lines.get_next_color()
Copy link
Member

Choose a reason for hiding this comment

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

when did we add cycling support directly into scatter3d? I would have let the 2D scatter handle that.

Copy link
Member

Choose a reason for hiding this comment

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

Went in in 48d5b6b

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see why (need to handle the None case). However, it should have still deferred to the 2d scatter. So, for the merge, have it so that it accepts my changes from v1.5 over what is in master for this portion of the code. My version defers the handling of None to 2d scatter, which is the correct course of action.

@WeatherGod
Copy link
Member

Yeah, the color cycling support and my fix for scatter3d are now clashing:

ERROR: test suite for <class 'mpl_toolkits.tests.test_mplot3d.test_scatter3d_color'>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/plugins/multiprocess.py", line 788, in run
    self.setUp()
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/suite.py", line 292, in setUp
    self.setupContext(ancestor)
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/plugins/multiprocess.py", line 770, in setupContext
    super(NoSharedFixtureContextSuite, self).setupContext(context)
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/suite.py", line 315, in setupContext
    try_run(context, names)
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/nose/util.py", line 490, in try_run
    return func()
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/matplotlib-1.5.1+408.g0047290-py2.7-linux-x86_64.egg/matplotlib/testing/decorators.py", line 193, in setup_class
    cls._func()
  File "/home/travis/build/matplotlib/matplotlib/lib/mpl_toolkits/tests/test_mplot3d.py", line 119, in test_scatter3d_color
    color='r', marker='o')
  File "/home/travis/build/matplotlib/matplotlib/lib/mpl_toolkits/mplot3d/axes3d.py", line 2285, in scatter
    patches = Axes.scatter(self, xs, ys, s=s, c=c, *args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/matplotlib-1.5.1+408.g0047290-py2.7-linux-x86_64.egg/matplotlib/__init__.py", line 1848, in inner
    return func(ax, *args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/venv/lib/python2.7/site-packages/matplotlib-1.5.1+408.g0047290-py2.7-linux-x86_64.egg/matplotlib/axes/_axes.py", line 3857, in scatter
    raise ValueError("Supply a 'c' kwarg or a 'color' kwarg"
ValueError: Supply a 'c' kwarg or a 'color' kwarg but not both; they differ but their functionalities overlap.

Cycling needs to be defered to the 2d function because all of the input handling needs to be figured out first.

mdboom and others added 5 commits May 19, 2016 08:15
@mdboom
Revert 2c1bc6b for performance
27d1199
@mdboom
Use ( , ) instead of [] for indexing
2c5d292 
Fix matplotlib#6444
@mdboom
Rename operator[] to subarray...
7976970 
Since we now know the performance is so bad, this should discourage its
use and flag it as something special.

Also, use () in more places
@mdboom
Don't use contouring on the path unless necessary
aef86ad
@tacaswell
Merge pull request matplotlib#6450 from mdboom/slow-path
5bad764 
PRF: Speed up point_in_path and friends
@mdboom
Copy link
Member

mdboom commented May 20, 2016

Ditto what @WeatherGod said: deferring to the 2D makes sense.

@jenshnielsen
Copy link
Member Author

I will try getting to working on this in the weekend but if anyone wants to work on it before hand feel free to replace and close this

@jenshnielsen
Copy link
Member Author

@WeatherGod I think this should resolve the 3d scatter conflict correctly. @mdboom I had to resolve another conflict in _image.h due to the merge of #6450 but it seems to only be due to code removed so I think it should keep the improved performance

@tacaswell tacaswell merged commit 7a268b9 into matplotlib:v2.x May 21, 2016
@jenshnielsen jenshnielsen deleted the merge15xto2x branch May 21, 2016 20:52
@WeatherGod
Copy link
Member

yes, that conflict resolution looks good to me. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jenshnielsen @WeatherGod @mdboom @tacaswell @dopplershift