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

Fix Line2D function set_markersize so it doesn't fail if given a string ... #4393

Merged
merged 1 commit into from May 24, 2015

Conversation

moonshoes87
Copy link
Contributor

...instead of a float.

I ran into an issue in my wxPython GUI where plotting fails because set_markersize is passed a string instead of a float. I wasn't able to figure out where the string argument was coming from, but this simple fix gets rid of the problem.

Here is the end of the error traceback I was getting:

File "/Users/xxxx/Library/Enthought/Canopy_64bit/User/lib/python2.7/site-packages/matplotlib/lines.py", line 752, in draw
snap = renderer.points_to_pixels(self._markersize) >= snap
File "/Users/xxxx/Library/Enthought/Canopy_64bit/User/lib/python2.7/site-packages/matplotlib/backends/backend_agg.py", line 299, in points_to_pixels
return points*self.dpi/72.0
TypeError: can't multiply sequence by non-int of type 'float'

@WeatherGod
Copy link
Member

-1. Instead, I would rework this to check that the markersize argument makes sense and throw a ValueError early. That would then help users find out the source of their problem rather than continuing to mask the problem.

@tacaswell
Copy link
Member

There is recent precedence for cleaning things up in this way: #4327

I think laundering this input through a float isn't a bad idea. It lets strings that a are 'numbers' pass (which is odd, but ok), but will blow up at the correct place if the user passes in a dictionary or 'aardvark'.

@tacaswell
Copy link
Member

that said @moonshoes87 you should really find the bug in your gui which is spitting out strings instead of floats.

@tacaswell tacaswell added this to the proposed next point release milestone May 1, 2015
@WeatherGod
Copy link
Member

I am still against this. That particular PR was attempting to solve an actual problem in our deep-level codebase in a somewhat roundabout way. This PR is not solving any bug in our code and goes against the documented API. We are not javascript. A string that looks like a number is still a string. Instead, I would want to raise a ValueError here so that users can find their own bug more easily.

@tacaswell
Copy link
Member

We also don't want to have the code littered with "isinstance".

Part of this will get hidden if we do go with managed properties (which I
am becoming solidly convinced we need to, it is just a question if which
framework).

On Fri, May 1, 2015, 09:43 Benjamin Root notifications@github.com wrote:

I am still against this. That particular PR was attempting to solve an
actual problem in our deep-level codebase in a somewhat roundabout way.
This PR is not solving any bug in our code and goes against the documented
API. We are not javascript. A string that looks like a number is still a
string. Instead, I would want to raise a ValueError here so that users can
find their own bug more easily.


Reply to this email directly or view it on GitHub
#4393 (comment)
.

@efiring
Copy link
Member

efiring commented May 1, 2015

I started out solidly in Ben's camp on this, and I originally opposed float(arg) in #4327. My opposition has softened because of the simplicity argument: avoiding all the is_numlike checking. But I'm still uncomfortable, and have the sense we are on a slippery slope, and might not like what we find at the bottom of it.

tacaswell added a commit that referenced this pull request May 24, 2015
MNT: Line2D.set_markersize takes any input which can be coerced to float
@tacaswell tacaswell merged commit 389e697 into matplotlib:master May 24, 2015
@tacaswell
Copy link
Member

@efiring @WeatherGod I am merging this as-is because it is basically harmless and will likely be superseded by the traitlets related MEP in anycase.

@tacaswell
Copy link
Member

@moonshoes87 Thank you. Hopefully we will see more contributions from you in the future!

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

4 participants