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

ENH/TST: Refactor refguide-check, take 3 #20127

Merged
merged 41 commits into from
May 15, 2024
Merged

Conversation

ev-br
Copy link
Member

@ev-br ev-br commented Feb 21, 2024

Reference issue

Continues and closes gh-16391, supersedes and closes gh-19242

What does this implement/fix?

Continue gh-19242 by @Sheila-nk : refactor the doctesting part of the refguide-check into a standalone tool, use pytest for running and orchestration.

So this PR has essentially two parts:

The remaining TODOs follow #19242 (comment). What's relevant now is

  • finish plumbing the external tool as git submodule
  • activate checking the code in tutorials
  • drop the obsoleted part of refguide-check. I'd prefer to do it in a quick follow-up PR.

On the 'scpdt' tool side, we need to

  • get a better name (doctest_sp, doctest_scipy, scipy_doctest, something else?) --- doctest-scipy unless there are other suggestions
  • get out of my GH --- will propose to move it to the scipy GH org on the ML

The tool itself is in a bit of a flux at the moment, nothing blocking though. Here's the tracker which is being actively worked on: https://github.com/ev-br/scpdt/issues

The pytest plugin is written by @Sheila-nk with inputs from @melissawm and myself. Thank you Sheila and Melissa!

Additional information

I am starting to dislike tagging this all as doctesting. The goal is not ---never was and will never be --- to mix doctests into unit testing. The goal is to keep the examples in the docs current. If somebody comes up with a catchy name for this, I'm all ears :-).

Copy link
Member Author

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

Let me flag several tweaks to the docstrings. The fixes are mainly mechanical, may very well be completely wrong, so would appreciate a sanity check from subject experts.

@@ -322,7 +322,7 @@ def milp(c, *, integrality=None, bounds=None, constraints=None, options=None):

>>> A = np.array([[-1, 1], [3, 2], [2, 3]])
>>> b_u = np.array([1, 12, 12])
>>> b_l = np.full_like(b_u, -np.inf)
>>> b_l = np.full_like(b_u, -np.inf, dtype=float)
Copy link
Member Author

Choose a reason for hiding this comment

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

@mdhaber git blame points at you, so I'd ask about this change. Without it, running the example from the milp docstring generates a warning from numpy; adding a dtype above changes the result from [1.0, 2.0] to [2.0, 2.0]:

In [1]: import numpy as np

In [2]: c = -np.array([0, 1])

In [3]: A = np.array([[-1, 1], [3, 2], [2, 3]])
   ...: >>> b_u = np.array([1, 12, 12])
   ...: >>> b_l = np.full_like(b_u, -np.inf)
/home/br/mambaforge/envs/scipy-dev/lib/python3.10/site-packages/numpy/core/numeric.py:407: RuntimeWarning: invalid value encountered in cast
  multiarray.copyto(res, fill_value, casting='unsafe')

In [4]: from scipy.optimize import LinearConstraint
   ...: >>> constraints = LinearConstraint(A, b_l, b_u)

In [5]: integrality = np.ones_like(c)

In [6]: from scipy.optimize import milp
   ...: >>> res = milp(c=c, constraints=constraints, integrality=integrality)
   ...: >>> res.x
Out[6]: array([1., 2.])

@@ -230,7 +230,6 @@

>>> A = lil_array((1000, 1000))
>>> A[0, :100] = rand(100)
>>> A[1, 100:200] = A[0, :100]
Copy link
Member Author

Choose a reason for hiding this comment

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

@dschult had to remove this line because otherwise it fails and compains about 1D slices.

scipy/stats/qmc.py Outdated Show resolved Hide resolved
scipy/stats/sampling.py Show resolved Hide resolved
@lucascolley lucascolley added maintenance Items related to regular maintenance tasks and removed scipy.stats scipy.sparse.linalg scipy.sparse scipy.optimize scipy._lib scipy.sparse.csgraph CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure labels Feb 21, 2024
@ev-br
Copy link
Member Author

ev-br commented Feb 21, 2024

Testing dev.py options:

  • $ python dev.py test --doctests -j 4 seems to give all workers all tests instead of distributing them? Needs checking, can be done in a follow-up (refguide-check never supported this). Is tracked at pytest-xdist breaks warning filters scipy_doctest#116
  • $ python dev.py test --doctests -t path/to/file does not work. Can be done in a follow-up, as refguide-check never supported this.

One other bikeshed item is the name of the switch: dev.py test --doctests (current state) or dev.py test --doctests-only?

With regular pytest pytest --doctests means run both unit tests and doctests; Here dev.py test --doctests skips regular unit tests entirely. OTOH --doctests-only is more typing. Thoughts?

Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Really cool 🚀

I am starting to dislike tagging this all as doctesting. The goal is not ---never was and will never be --- to mix doctests into unit testing. The goal is to keep the examples in the docs current. If somebody comes up with a catchy name for this, I'm all ears :-).

We have smoke-tests, what about smoke-doc? 😅

scipy/stats/qmc.py Outdated Show resolved Hide resolved
@lucascolley
Copy link
Member

The commit attribution to me hasn't quite worked due to extra quotation marks, but no biggie :) FWIW I'm a fan of smoke-doc

Comment on lines +77 to +106
"--ignore=scipy/interpolate/_interpnd_info.py",
"--ignore=scipy/_lib/array_api_compat",
"--ignore=scipy/_lib/highs",
"--ignore=scipy/_lib/unuran",
"--ignore=scipy/_lib/_gcutils.py",
"--ignore=scipy/_lib/doccer.py",
"--ignore=scipy/_lib/_uarray",
Copy link
Member

Choose a reason for hiding this comment

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

can we get rid of the errors from the files which aren't submodules / vendored?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably. Together with scipy/scipy_doctest#94
I'd defer it to a follow-up though.

Copy link
Member Author

Choose a reason for hiding this comment

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

That said, if you feel like tackling it in this PR Lucas, I won't mind at all :-).

Copy link
Member

Choose a reason for hiding this comment

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

don't tempt me... (no promises but maybe)

ev-br added 21 commits May 15, 2024 11:30
Some of them are broken (io.rst? sampling_XXX?); others need a tooling update.

This way, the output of

$ pytest doc/source/tutorial/ --doctest-glob=*rst --ignore=doc/source/tutorial/examples

is green.
While at it, use csr_array not csr_matrix (cosmetics).
Had to remove Ellipsis matching on numeric values in several
places.

[docs only]
Doctestrings were updated for NumPy 2.0, scalar reprs have changed.
This is a minimal fix, just to avoid `dev.py refguide-check` erroring out.

[docs only]
@ev-br ev-br force-pushed the doctest_plugin branch 2 times, most recently from 9d2311f to fff11c2 Compare May 15, 2024 10:01
@ev-br
Copy link
Member Author

ev-br commented May 15, 2024

No further comments, CI is happy, the plugin installs from PyPI -- let's roll with this.

A huge thank you to Sheila @Sheila-nk for doing the heavy lifting, and thanks to all reviewers.

@ev-br ev-br merged commit 93e0aec into scipy:main May 15, 2024
28 checks passed
ev-br added a commit to ev-br/scipy that referenced this pull request May 15, 2024
Use $ python dev.py smoke-docs and dev.py smoke-tutorial instead
Both ways of running doctests, smoke-docs and refguide-check,
are avaiable in scipy#20127.

[docs only]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement maintenance Items related to regular maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants