Skip to content

FIX: Fix get_facecolors #11877

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

Closed
wants to merge 1 commit into from
Closed

Conversation

larsoner
Copy link
Contributor

@larsoner larsoner commented Aug 18, 2018

On master the added test throws:

lib/mpl_toolkits/tests/test_mplot3d.py:261: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
lib/matplotlib/cbook/__init__.py:1918: in method
    return getattr(self, name)(*args, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <mpl_toolkits.mplot3d.art3d.Poly3DCollection object at 0x7f80a4da6f60>

    def get_facecolor(self):
>       return self._facecolors2d
E       AttributeError: 'Poly3DCollection' object has no attribute '_facecolors2d'

Now it passes.

I also added a r""" on a docstring in lib/matplotlib/artist.py that had a \s in it as it was raising a SyntaxError on my system without it.

@jklymak
Copy link
Member

jklymak commented Aug 18, 2018

I'm confused by this. Are you saying get_facecolor doesn't work on master? If so, why are you adding a new method, get_facecolors that returns something different? If this is indeed the right thing to do, then should you also add get_edgecolors?

@ImportanceOfBeingErnest
Copy link
Member

Might be related: #10797

@larsoner
Copy link
Contributor Author

Are you saying get_facecolor doesn't work on master?

Yes indeed for PolyCollection3D, the code snippet in my top comment that shows what the smoke test does on master.

If so, why are you adding a new method, get_facecolors that returns something different

The method from the super class PolyCollection tries to return _facecolors2d which does not necessarily exist. I looked and saw _facecolors did so I used that, but it's possible:

  1. it should be _facecolors3d
  2. some other function needs to be called to ensure that some magic happens so that _facecolors2d does exist. It looks like _facecolors2d should be set by do_3d_projection, so perhaps that isn't being called but should be. The Poly3DCollection code has a comment that it does some magic with these properties, so I'm not really sure which is correct. I should double-check which one makes our code work properly.
  3. Something else entirely.

I have not done git blame but based on how long I've been hitting this issue (months, not years) and browsing the GitHub history, it looks like this probably caused it:

7c40fdf

I'll try to do a proper git blame at some point.

If this is indeed the right thing to do, then should you also add get_edgecolors

Yes this is true, I'll add tests for a few other properties.

Might be related: #10797

#10797 looks related, but I would be surprised if it fixed this bug. But the magical aspects of Poly3DCollection might allow it to fix the bug in some way that is not immediately obvious.

@larsoner
Copy link
Contributor Author

I probably should have tackled this PR sooner -- we now get failures in our master code, so I guess this is in the newly released 2.2.3, too:

https://travis-ci.org/mne-tools/mne-python/jobs/417532117#L2699

@ImportanceOfBeingErnest
Copy link
Member

Also see #9559. At the end this is the same as #4067. There are some comments by @WeatherGod and an attempted fix in #4090 which never made it through. Not sure what problems there were/are.

@larsoner
Copy link
Contributor Author

I guess this should be closed in favor of #4090, then?

@jklymak
Copy link
Member

jklymak commented Aug 18, 2018

Yes indeed for PolyCollection3D, the code snippet in my top comment that shows what the smoke test does on master.

I'm still confused. Your smoke test calls get_facecolors, whereas the error above is for get_facecolor (no s at the end). If so, I don't see how the PR gets rid of the original error message.

ping @WeatherGod

@larsoner
Copy link
Contributor Author

Ahh I see what you mean. In the older code / base I was working on, the plural was just an alias for the non-plural in PolyCollection. In master I can't find the def of get_facecolors anywhere, despite it being used in several examples and even in collections.py itself. So it's either just implicitly an alias now, or there are some more bugs / untested lines and examples.

In any case I'll close this for now since I found a suitable workaround, and #4090 is probably the right way to go anyway.

@larsoner larsoner closed this Aug 18, 2018
@larsoner larsoner deleted the fix-mplot3d branch August 18, 2018 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants