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

Cast to integer to get rid of numpy warning #3241

Merged
merged 11 commits into from Aug 26, 2014

Conversation

jenshnielsen
Copy link
Member

No description provided.

@NelleV
Copy link
Member

NelleV commented Jul 12, 2014

Can you please squash the commits? If you've never done this, you can ask one of the git guru experts.

@tacaswell
Copy link
Member

Given the nature of these changes I think it is better to leave this un-squashed to make bisect work well.

@NelleV
Copy link
Member

NelleV commented Jul 12, 2014

These are all integer casting. If a bug occurs, it should be pretty straightforward to identify which line it comes from.

@tacaswell tacaswell added this to the v1.4.x milestone Jul 12, 2014
@jenshnielsen
Copy link
Member Author

@NelleV I discussed this with @tacaswell The issue is that these are all over the code base in widely different parts of the codebase.

@NelleV
Copy link
Member

NelleV commented Jul 12, 2014

I guess this is a philosophical questions, but I am afraid we are going to end up with twenty of these one liner commits. They all solve one problem, thus could be in a single commits. Moreover, the commits messages makes it really hard to distinguish between all of these (thus the squash comment).
I don't feel very strongly about this, but IMO this is a case study of the use case of squashing commits.

@jenshnielsen
Copy link
Member Author

This is not a case of that I don't want to rebase we just don't think that it is the right solution.

Rewrote to use better commit messages.

@argriffing
Copy link

The contour.py change in this PR tries to int-ify things that are compared vs. nan for control flow later.

Also I think the cbook.py int-ification is redundant with a change later in that function which could be removed.

Anyway I agree with the idea of making changes like these, and I also did most of them in #3289 before I found this PR. I didn't do the ones in hatch.py because they could be more subtle. I don't care which PR is accepted or how the commits are squashed or separated..

@jenshnielsen
Copy link
Member Author

Thanks @argriffing You are right. I did not look closely enough at that piece of code but trusted the comment above the line about casting. I have updated this pull request with your fixes. The fact that this didn't fail indicates that we are missing a unit test to cover these cases. This would be good to add if possible.

This pull request fixes all the warnings that are raised during the test suite. However, it is not unlikely that there are more in parts of the code not unit tested. If anyone spots them they should be reported at the Github bug tracker. I guess we will only really know when numpy starts turning these into real errors.

Why do you think the changes in hatch.py ar subtle? One can delay the cast until the actual indexing but it seems to me that the number of lines must be an integer pr definition.

@argriffing
Copy link

Why do you think the changes in hatch.py ar subtle?
One can delay the cast until the actual indexing but it seems to me that the number of lines
must be an integer pr definition.

You are probably right. Everything is much more straightforward when you believe comments and variable names!

@tacaswell
Copy link
Member

Closes #3137

tacaswell added a commit that referenced this pull request Aug 26, 2014
Cast to integer to get rid of numpy warning
@tacaswell tacaswell merged commit 9a42b8d into matplotlib:master Aug 26, 2014
tacaswell added a commit that referenced this pull request Aug 26, 2014
Cast to integer to get rid of numpy warning
@tacaswell
Copy link
Member

cherry-picked as 3bc07d1

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