-
Notifications
You must be signed in to change notification settings - Fork 441
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
decrease tol for camera test due to MacOS #2836
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2836 +/- ##
=======================================
Coverage 94.03% 94.03%
=======================================
Files 76 76
Lines 16399 16399
=======================================
Hits 15420 15420
Misses 979 979 |
I'm not quite sure whether this is a MacOS thing or some pathological combination of random numbers. If the latter, we could use specified values for the test. |
There's some really creepy thing going on. Notice that the error comes from this assert: camera.distance = distance
assert np.isclose(camera.distance, distance) In any sane setup I'd expect those two be very close! Instead this is what I see, on Linux: >>> import pyvista as pv
>>> cam = pv.Camera()
>>> cam.distance
1.0
>>> cam.distance = 0.00016663273706207793
>>> cam.distance
0.0002
>>> type(cam.distance)
<class 'float'>
>>> cam.distance - 0.00016663273706207793
3.336726293792208e-05 What's up with this?? This can only be VTK's fault, right? Considering pyvista/pyvista/plotting/camera.py Lines 225 to 228 in 36ab8b5
|
VTK seems to set a hard lower bound... >>> cam.distance = 0.001
>>> cam.distance
0.001
>>> cam.distance = 0.0001
>>> cam.distance
0.0002
>>> cam.distance = 0.00001
>>> cam.distance
0.0002 |
Good catch. It indeed has a limit: This confirms that it is a pathological random number. We should pick a number greater than this limit. |
This works: >>> np.isclose(0.0002, 0, atol=0.0002)
True Since the smallest possible value for |
Yeah, I think this can work. |
Co-authored-by: Andras Deak <adeak@users.noreply.github.com>
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.
LGTM!
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.
Let's see if re-approving adds the label.
Edit: nevermind, the labelling action is not merged yet, right?
It is, but it needs an additional PR. |
We recently had a test failure in this run.
This PR simply increases the tolerance for this.