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 compare bug #22

Merged
merged 5 commits into from
Mar 7, 2017
Merged

fix compare bug #22

merged 5 commits into from
Mar 7, 2017

Conversation

marqh
Copy link
Member

@marqh marqh commented Feb 23, 2017

this is a subtle bug

for certain configurations of python and numpy

calendars[0] == calendars

returns False, whilst

calendars == calendars[0]

returns array([ True, True,], dtype=bool)

therefore

np.all(calendars[0] == calendars)

returns False, whilst

np.all(calendars == calendars[0])

returns True

as type coercion, where it occurs, tends to happen left to right, this order is safer

@marqh marqh requested review from DPeterK and lbdreyer February 23, 2017 21:31
@DPeterK
Copy link
Member

DPeterK commented Feb 24, 2017

type coercion, where it occurs

Maybe it would be better to avoid type coercion entirely?

@marqh
Copy link
Member Author

marqh commented Feb 28, 2017

i'm struggling to understand these test failures. These tests worked before and after the last change was proposed and merged

i can change the test results, but I'm not followiing why that should be required
the matplotlib versions are the same, i don't know what else could lead to this discrepancy

@coveralls
Copy link

Coverage Status

Coverage remained the same at 90.566% when pulling e88bb1e on marqh:compare into d64de36 on SciTools:master.

@marqh
Copy link
Member Author

marqh commented Feb 28, 2017

@bjlittle please may you have a quick look and see if any clues about test changes jump out?

@tv3141
Copy link

tv3141 commented Mar 2, 2017

@marqh To me it seems the matplotlib version actually did change.

The Travis logs in this pull request show that matplotlib 2.0.0 was used, while the Travis logs of the last successful run, pull request #19 use matplotlib 1.5.1.

Btw Travis did fail in the previous pull request #21, but it shows as succeeded in the pull request itself.

@marqh
Copy link
Member Author

marqh commented Mar 3, 2017

To me it seems the matplotlib version actually did change.

that would explain the test changes

thank you
@tv3141

@coveralls
Copy link

Coverage Status

Coverage remained the same at 90.566% when pulling 580019b on marqh:compare into d64de36 on SciTools:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 90.566% when pulling 580019b on marqh:compare into d64de36 on SciTools:master.

@marqh marqh mentioned this pull request Mar 3, 2017
Closed
@marqh marqh merged commit b1c790b into SciTools:master Mar 7, 2017
@DPeterK
Copy link
Member

DPeterK commented Mar 7, 2017

@marqh please note that @lbdreyer and @bjlittle had not approved this PR. The fact that I had does not mean this PR was necessarily merge-ready.

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.

4 participants