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 lw float cast #4327

Merged
merged 3 commits into from Apr 13, 2015
Merged

Fix lw float cast #4327

merged 3 commits into from Apr 13, 2015

Conversation

tacaswell
Copy link
Member

there is a set_linewidth in backend_gdk.py, but it calls the parent class set_linewdith which already has the needed cast.

Explicitly cast linewidth to a float in `set_linewidth` methods
of `Line2D` and `Patch`

Closes matplotlib#4306
@tacaswell tacaswell added this to the next point release milestone Apr 12, 2015
@tacaswell
Copy link
Member Author

Closes #4306

@efiring
Copy link
Member

efiring commented Apr 12, 2015

This looks reasonable and will do the job--but I'm still uncomfortable with the idea of supporting plot(x, y, lw='1.5'). To me, this is just bad code that is slipping through because we have placed a low priority on argument validation, it is contrary to the documentation, and therefore it is not something we have to support in the future--it was only half-supported by accident. I don't see any use-case justification for it.

@tacaswell
Copy link
Member Author

On the other hand, supporting 'float like' seems very much in the spirit of python and duck typing. This will also clean up any issues with scalar numpy arrays or integers going through.

@efiring
Copy link
Member

efiring commented Apr 12, 2015

A string does not behave like a number, so no, it is not in the spirit of duck typing. A string is not "float-like" at all. It doesn't pass our is_numlike() duck-typing test. I don't object to using the float cast if it helps homogenize things that would otherwise cause trouble in the lower levels, but I don't like the idea of thinking of a string representation of a float as equivalent to a float. In color spec parsing a string representation of a float is used for gray; the distinction between the string and a true number is critical.

@tacaswell
Copy link
Member Author

Fair enough, I was using 'float-like' <-> float(foo) does not fail.

I had forgotten that detail in the colorspec.

The alternative I see is

if is_numlike(lw):
    lw = float(lw)  # still want to homogenize for down stream I think
else:
    raise ValueError("...")

My inclination is to be as permissive as possible, but will change them all over to this form if you want.

@efiring
Copy link
Member

efiring commented Apr 13, 2015

This brings us to some strategy questions regarding input argument validation that should be addressed but can be deferred. For the time being I don't mind leaving the PR as-is; but I don't want it to be seen as endorsing or even officially supporting string inputs where numbers are called for.

efiring added a commit that referenced this pull request Apr 13, 2015
@efiring efiring merged commit 5d06a35 into matplotlib:master Apr 13, 2015
@tacaswell tacaswell deleted the fix_lw_float_cast branch April 13, 2015 00:55
@tacaswell
Copy link
Member Author

Fully on board with not endorsing string input for linewidth and would have no problem breaking this API with out warning (so long as we push the exception way up the interaction stack).

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.

None yet

2 participants