-
Notifications
You must be signed in to change notification settings - Fork 55
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 test_packages failures #519
Conversation
…ese changes seems agains the spirit of the test and warrants investigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for the fixes!
@coreyostrove I actually haven't finished the third failure yet |
Whoops! I rescind the approval in that case, my bad. |
I'd like #517 to be merged before finishing / merging in this PR. |
#517 is in now :) |
Excellent. I've launched the tests again. If they pass then I'll remove the WIP prefix and be okay with a merge. |
Tests passed! Of course, we already discussed how these test failures didn't reliably manifest on other branches, but this PR includes formally correct fixes. It's reasonable to merge as-is. |
pygsti/data/dataset.py
Outdated
if rep_array is not None: | ||
dtype_info = _np.finfo(rep_array.dtype) | ||
near_zero = rep_array < 10**(-1.5*dtype_info.precision) | ||
rep_array[near_zero] = 0 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to double-check, this was setting things that are numerically 0 to 0 so that line 1741 drops it if record_zero_counts is False? If so, can we add that as a comment? I don't work with the array mask syntax much and this took me a second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! Comment added :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Also, just a random sidenote: This looked much more natural to me in 515 where it was just array[array < 1e-10] = 0... I don't know why having the mask split out on its own line took me by surprise 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect thanks!
Running
pytest test/test_packages
on the latest develop branch results in errors. Summary output:I've dug into each of these.
Failures in
test/test_packages/drivers/test_timedep.py
are related to the use of float32 to hold count data in dataset.py. After the 12/17 dev meeting I implemented a proper fix.Failures in
test/test_packages/objects/test_evaltree.py
were very mild. I implemented fixes in this PR but removed them since fixes are already present in #517.All that remains are failures in
test/test_packages/drivers/test_drivers.py
.Click me for details on those failures.