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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix UniformGrid.x docstring #2511

Merged
merged 1 commit into from
Apr 25, 2022
Merged

Conversation

MatthewFlamm
Copy link
Contributor

@MatthewFlamm MatthewFlamm commented Apr 25, 2022

Overview

When implementing #2510, this bug was found. I'm not sure how this could pass the current doctest since it compares the strings 馃し

@github-actions github-actions bot added the bug Uh-oh! Something isn't working as expected. label Apr 25, 2022
Copy link
Member

@adeak adeak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

I'm not sure how this could pass the current doctest since it compares the strings 馃し

That's reaaaly creepy. What also passes:

>>> grid.y
array([0., 1., 0., 1., 0., 1., 0., 2.])

But not

>>> grid.y
array([0., 1., 0., 1., 0., 1., 0., 3.])

Or

>>> grid.y
array([0., 1., 0., 1., 0., 1., 0., -2.])

Approving this fix for now, but we should really get to the bottom of this.

@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #2511 (9db5e45) into main (9a79214) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2511      +/-   ##
==========================================
+ Coverage   93.70%   93.77%   +0.06%     
==========================================
  Files          75       75              
  Lines       16072    16072              
==========================================
+ Hits        15061    15071      +10     
+ Misses       1011     1001      -10     

@adeak
Copy link
Member

adeak commented Apr 25, 2022

Oh wow, each item is allowed to have an absolute difference of 1 with respect to the true value:

>>> grid.x
array([-1., 2., -1., 2., -1., 2., -1., 2.])

鈽濓笍 this runs fine.

@MatthewFlamm
Copy link
Contributor Author

MatthewFlamm commented Apr 25, 2022

Oh wow, each item is allowed to have a difference of 1 with respect to the true value:

>>> grid.x
array([-1., 2., -1., 2., -1., 2., -1., 2.])

鈽濓笍 this runs fine.

馃く This is really really bad. I've found an MRE.

"""
Examples
--------
>>> import numpy as np
>>> np.array([0.])  # doctest: +NUMBER
array([1.])
"""

I wonder if this is why pytest-doctestplus disables the NUMBER directive?

@MatthewFlamm
Copy link
Contributor Author

Also not limited to numpy arrays. Ouch.

"""
>>> [0.]  # doctest: +NUMBER
[1.]
"""

@adeak
Copy link
Member

adeak commented Apr 25, 2022

Also not limited to numpy arrays. Ouch.

At least that makes sense to me. The only thing here that makes sense to me 馃檮

@MatthewFlamm
Copy link
Contributor Author

Now that we can confirm that it isn't a pyvista specific issue, I think we are good to merge this. I will create an issue in pytest repo.

@MatthewFlamm MatthewFlamm merged commit 04516fe into main Apr 25, 2022
@MatthewFlamm MatthewFlamm deleted the fix-uniformgrid-docstring-example branch April 25, 2022 20:28
@MatthewFlamm
Copy link
Contributor Author

To close the loop here, the NUMBER option does not just compare up to the precision given as indicated in the documentation. It uses pytest.approx to compare using the precision as the relative tol. So 1. and 0. are compared as 0. == approx(1., rel=1.). This results in evaluating to True.

If pytest chooses to keep this behavior, we should drop NUMBER as a by default doctest option. This case is unexpected and will be too common in this type of codebase.

@MatthewFlamm
Copy link
Contributor Author

I always wondered why sometimes it seemed that our doctest was inconsistent on when it rounded numbers, and now I know why.

adeak added a commit to adeak/pyvista that referenced this pull request Apr 28, 2022
* upstream/main:
  Make VTK version error clear when PointSet is still abstract (pyvista#2483)
  Use imageio intersphinx links (pyvista#2489)
  Fix glyphs when orienting with cell data (pyvista#2500)
  Bump mypy from 0.942 to 0.950 (pyvista#2522)
  Update hypothesis requirement from <6.45.1 to <6.45.2 (pyvista#2523)
  Add many Readers (pyvista#2496)
  Bump trimesh from 3.10.8 to 3.11.2 (pyvista#2519)
  Return actor from add_mesh_threshold (pyvista#2516)
  fix uniformgrid.x docstring (pyvista#2511)
  Update imageio requirement from <2.18.0 to <2.19.0 (pyvista#2506)
  Update hypothesis requirement from <6.44.1 to <6.45.1 (pyvista#2507)
  add polyhedral example (pyvista#2505)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Uh-oh! Something isn't working as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants