Use scipy-doctest instead of refguide-check - #747

Merged
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs
Jun 25, 2024
Merged

Use scipy-doctest instead of refguide-check#747
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs

Conversation

@ev-br

@ev-brev-br commented Jun 2, 2024

Copy link
Copy Markdown
Contributor

SciPy has recently refactored its refguide-check utility into a separate pip-installable package and added integration of the modified doctesting with pytest. This PR switches the refguide-check run (which uses the utility vendored from scipy) to this refactored way.

There are several minor tweaks to docstrings --- I'm actually not sure all the affected docstrings were checked previously (but they are now):

  • Explicit $ doctest: comments are no longer needed
  • A refactor of the ContinuousWavelet example is to avoid a DeprecationWarning from matplotlib 3.7
  • Some whitespace tweaks where the printing is non-standard enough for the tool to give up and fall back to the (whitespace-sensitive) vanilla doctest checker.

The pytest incantations to run doctesting are explicit on the CI --- in SciPy, these are hidden behind the dev.py smoke-docs interface.

ev-br added 3 commits June 2, 2024 18:58
$ pytest --doctest-modules --pyargs pywt -v --doctest-collect=api
...
pywt/__init__.py::pywt.ContinuousWavelet.wavefun PASSED [ 3%]
pywt/__init__.py::pywt.Modes PASSED [ 6%]
pywt/__init__.py::pywt.Wavelet.wavefun PASSED [ 9%]
pywt/__init__.py::pywt.array_to_coeffs PASSED [ 12%]
pywt/__init__.py::pywt.coeffs_to_array PASSED [ 16%]
pywt/__init__.py::pywt.cwt PASSED [ 19%]
pywt/__init__.py::pywt.dwt PASSED [ 22%]
pywt/__init__.py::pywt.dwt2 PASSED [ 25%]
pywt/__init__.py::pywt.dwt_max_level PASSED [ 29%]
pywt/__init__.py::pywt.dwtn_max_level PASSED [ 32%]
pywt/__init__.py::pywt.families PASSED [ 35%]
pywt/__init__.py::pywt.fswavedecn PASSED [ 38%]
pywt/__init__.py::pywt.idwt PASSED [ 41%]
pywt/__init__.py::pywt.idwt2 PASSED [ 45%]
pywt/__init__.py::pywt.integrate_wavelet PASSED [ 48%]
pywt/__init__.py::pywt.iswt PASSED [ 51%]
pywt/__init__.py::pywt.iswt2 PASSED [ 54%]
pywt/__init__.py::pywt.iswtn PASSED [ 58%]
pywt/__init__.py::pywt.ravel_coeffs PASSED [ 61%]
pywt/__init__.py::pywt.threshold PASSED [ 64%]
pywt/__init__.py::pywt.unravel_coeffs PASSED [ 67%]
pywt/__init__.py::pywt.upcoef PASSED [ 70%]
pywt/__init__.py::pywt.wavedec PASSED [ 74%]
pywt/__init__.py::pywt.wavedec2 PASSED [ 77%]
pywt/__init__.py::pywt.wavedecn PASSED [ 80%]
pywt/__init__.py::pywt.wavedecn_shapes PASSED [ 83%]
pywt/__init__.py::pywt.wavedecn_size PASSED [ 87%]
pywt/__init__.py::pywt.wavelist PASSED [ 90%]
pywt/__init__.py::pywt.waverec PASSED [ 93%]
pywt/__init__.py::pywt.waverec2 PASSED [ 96%]
pywt/__init__.py::pywt.waverecn PASSED [100%]
================================== 31 passed in 0.80s =====================
Makes this green:
$ pytest --doctest-glob=*rst doc/source/regression/ -vs
@ev-brev-br changed the title Use Use scipy-doctest instead of refguide-checkJun 2, 2024

@agriyakhetarpalagriyakhetarpal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing this, @ev-br! All of the changes look good to me. I would suggest leaving out the files under doc/source/regression/, though – they are being converted to Markdown-style notebooks in gh-741, which means that the changes to them won't be needed since MyST-NB will execute them. That PR is close to getting merged and should be done by today or so.

ev-br added 2 commits June 3, 2024 18:15
The script can do two things:
- run modified doctests; this is done via smoke-docs / scipy-doctests now
- check __all__ lists vs refguide entries; this was already disabled, on CI at least
@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @agriyakhetarpal . Rolled back all changes to *rst files, removed the corresponding stanza from the CI run, and removed the refguide-check utility as "not used anymore". Would be nice if you could trigger the CI run for me, would you?

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

Thanks! I tried but it turns out I don't have enough permissions to trigger a CI run – tagging @rgommers who can do this for you.

@rgommers

Copy link
Copy Markdown
Member

triggered now - the joys of spammer-protection:)

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks Ralf! So the CI is green, thus the question is whether y'all want it :-). And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

@rgommers

Copy link
Copy Markdown
Member

Yes, and yes. thanks:)

And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

Do you want to include that here, or separately?

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

A follow-up PR would not require restarting the CI for me, so seems easier :-).

@rgommers

Copy link
Copy Markdown
Member

Did you actually notice the doctesting happening in CI? I only see REFGUIDE_CHECK: [0] in test.yml.

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Indeed! So it was never actually running on CI, huh :-). Pushed a commit to switch it always on. Depending on how you want to play this, can iterate the CI in follow-ups or here. (If the latter, a new commit asks for one more confirmation I'm not a crypto miner)

@rgommers

Copy link
Copy Markdown
Member
ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'util/readthedocs/requirements.txt'

If it becomes a pain for CI not to run, maybe submit a trivial typo/change PR that can be merged straight away?

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

With the latest failure, maybe we should do cd .. rather, much easier than fixing the paths in-place every time.

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@ev-br

ev-br commented Jun 4, 2024

Copy link
Copy Markdown
ContributorAuthor

Since sphinx on CI, which is the source of the failures, is not really related, maybe it's best to separate the fixes.

Comment thread.github/workflows/tests.yml Outdated
Comment thread.github/workflows/tests.yml Outdated
@ev-br

ev-br commented Jun 6, 2024

Copy link
Copy Markdown
ContributorAuthor

Okay, current status:

  • smoke-docs runs fine on CI
  • sphinx-build was disabled on CI, is still disabled on CI.
  • attempting to run sphinx-build locally shows a bunch of errors, so there's some structural problem with it.
  • However, docs build on readthedocs. Meaning, the problem is with the sphinx-build invocation which did not run on CI previously.

So how about not fixing the sphinx-build check in this PR? If that's OK, the PR is ready from my side.

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

I'll hope to fix the sphinx-build checks in a separate PR after this – I would be fine with this getting merged.

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ev-br, this looks quite nice! Overall much cleaner docs, in addition to getting rid of the refguide_check.py script which is great. Just a few small comments.

Comment threadpywt/data/_readers.py Outdated
Comment threadpywt/_extensions/_pywt.pyx Outdated
pip install -r util/readthedocs/requirements.txt
sphinx-build -b html -W --keep-going -d _build/doctrees . doc/source doc/build
cd ..
# XXX sphinx build is broken on CI

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is happening here? Debug left-over, or is something actually broken right now? If so, I think three lines can be safely deleted. The separate CI job to build the docs with the ReadTheDocs integration is green.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The RTD job is not building the docs with -W, I guess: python -m sphinx -T -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html. Not that there are any warnings because we're already warning-free.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Indeed, this sphinx-build incantation did not previously run on CI because REFGUIDE-CHECK: [0], and was always broken. There's a standing offer to fix this in a follow-up though :-). #747 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay thanks, let's do that - offer is appreciated:)

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM now and CI is happy, so in it goes. Thanks @ev-br! And thanks @agriyakhetarpal for the review and help.

@rgommers
rgommers merged commit f2b3d58 into PyWavelets:mainJun 25, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ev-br@agriyakhetarpal@rgommers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Use scipy-doctest instead of refguide-check - #747

Merged
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs
Jun 25, 2024
Merged

Use scipy-doctest instead of refguide-check#747
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs

Conversation

@ev-br

@ev-brev-br commented Jun 2, 2024

Copy link
Copy Markdown
Contributor

SciPy has recently refactored its refguide-check utility into a separate pip-installable package and added integration of the modified doctesting with pytest. This PR switches the refguide-check run (which uses the utility vendored from scipy) to this refactored way.

There are several minor tweaks to docstrings --- I'm actually not sure all the affected docstrings were checked previously (but they are now):

  • Explicit $ doctest: comments are no longer needed
  • A refactor of the ContinuousWavelet example is to avoid a DeprecationWarning from matplotlib 3.7
  • Some whitespace tweaks where the printing is non-standard enough for the tool to give up and fall back to the (whitespace-sensitive) vanilla doctest checker.

The pytest incantations to run doctesting are explicit on the CI --- in SciPy, these are hidden behind the dev.py smoke-docs interface.

ev-br added 3 commits June 2, 2024 18:58
$ pytest --doctest-modules --pyargs pywt -v --doctest-collect=api
...
pywt/__init__.py::pywt.ContinuousWavelet.wavefun PASSED [ 3%]
pywt/__init__.py::pywt.Modes PASSED [ 6%]
pywt/__init__.py::pywt.Wavelet.wavefun PASSED [ 9%]
pywt/__init__.py::pywt.array_to_coeffs PASSED [ 12%]
pywt/__init__.py::pywt.coeffs_to_array PASSED [ 16%]
pywt/__init__.py::pywt.cwt PASSED [ 19%]
pywt/__init__.py::pywt.dwt PASSED [ 22%]
pywt/__init__.py::pywt.dwt2 PASSED [ 25%]
pywt/__init__.py::pywt.dwt_max_level PASSED [ 29%]
pywt/__init__.py::pywt.dwtn_max_level PASSED [ 32%]
pywt/__init__.py::pywt.families PASSED [ 35%]
pywt/__init__.py::pywt.fswavedecn PASSED [ 38%]
pywt/__init__.py::pywt.idwt PASSED [ 41%]
pywt/__init__.py::pywt.idwt2 PASSED [ 45%]
pywt/__init__.py::pywt.integrate_wavelet PASSED [ 48%]
pywt/__init__.py::pywt.iswt PASSED [ 51%]
pywt/__init__.py::pywt.iswt2 PASSED [ 54%]
pywt/__init__.py::pywt.iswtn PASSED [ 58%]
pywt/__init__.py::pywt.ravel_coeffs PASSED [ 61%]
pywt/__init__.py::pywt.threshold PASSED [ 64%]
pywt/__init__.py::pywt.unravel_coeffs PASSED [ 67%]
pywt/__init__.py::pywt.upcoef PASSED [ 70%]
pywt/__init__.py::pywt.wavedec PASSED [ 74%]
pywt/__init__.py::pywt.wavedec2 PASSED [ 77%]
pywt/__init__.py::pywt.wavedecn PASSED [ 80%]
pywt/__init__.py::pywt.wavedecn_shapes PASSED [ 83%]
pywt/__init__.py::pywt.wavedecn_size PASSED [ 87%]
pywt/__init__.py::pywt.wavelist PASSED [ 90%]
pywt/__init__.py::pywt.waverec PASSED [ 93%]
pywt/__init__.py::pywt.waverec2 PASSED [ 96%]
pywt/__init__.py::pywt.waverecn PASSED [100%]
================================== 31 passed in 0.80s =====================
Makes this green:
$ pytest --doctest-glob=*rst doc/source/regression/ -vs
@ev-brev-br changed the title Use Use scipy-doctest instead of refguide-checkJun 2, 2024

@agriyakhetarpalagriyakhetarpal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing this, @ev-br! All of the changes look good to me. I would suggest leaving out the files under doc/source/regression/, though – they are being converted to Markdown-style notebooks in gh-741, which means that the changes to them won't be needed since MyST-NB will execute them. That PR is close to getting merged and should be done by today or so.

ev-br added 2 commits June 3, 2024 18:15
The script can do two things:
- run modified doctests; this is done via smoke-docs / scipy-doctests now
- check __all__ lists vs refguide entries; this was already disabled, on CI at least
@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @agriyakhetarpal . Rolled back all changes to *rst files, removed the corresponding stanza from the CI run, and removed the refguide-check utility as "not used anymore". Would be nice if you could trigger the CI run for me, would you?

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

Thanks! I tried but it turns out I don't have enough permissions to trigger a CI run – tagging @rgommers who can do this for you.

@rgommers

Copy link
Copy Markdown
Member

triggered now - the joys of spammer-protection:)

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks Ralf! So the CI is green, thus the question is whether y'all want it :-). And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

@rgommers

Copy link
Copy Markdown
Member

Yes, and yes. thanks:)

And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

Do you want to include that here, or separately?

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

A follow-up PR would not require restarting the CI for me, so seems easier :-).

@rgommers

Copy link
Copy Markdown
Member

Did you actually notice the doctesting happening in CI? I only see REFGUIDE_CHECK: [0] in test.yml.

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Indeed! So it was never actually running on CI, huh :-). Pushed a commit to switch it always on. Depending on how you want to play this, can iterate the CI in follow-ups or here. (If the latter, a new commit asks for one more confirmation I'm not a crypto miner)

@rgommers

Copy link
Copy Markdown
Member
ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'util/readthedocs/requirements.txt'

If it becomes a pain for CI not to run, maybe submit a trivial typo/change PR that can be merged straight away?

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

With the latest failure, maybe we should do cd .. rather, much easier than fixing the paths in-place every time.

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@ev-br

ev-br commented Jun 4, 2024

Copy link
Copy Markdown
ContributorAuthor

Since sphinx on CI, which is the source of the failures, is not really related, maybe it's best to separate the fixes.

Comment thread.github/workflows/tests.yml Outdated
Comment thread.github/workflows/tests.yml Outdated
@ev-br

ev-br commented Jun 6, 2024

Copy link
Copy Markdown
ContributorAuthor

Okay, current status:

  • smoke-docs runs fine on CI
  • sphinx-build was disabled on CI, is still disabled on CI.
  • attempting to run sphinx-build locally shows a bunch of errors, so there's some structural problem with it.
  • However, docs build on readthedocs. Meaning, the problem is with the sphinx-build invocation which did not run on CI previously.

So how about not fixing the sphinx-build check in this PR? If that's OK, the PR is ready from my side.

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

I'll hope to fix the sphinx-build checks in a separate PR after this – I would be fine with this getting merged.

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ev-br, this looks quite nice! Overall much cleaner docs, in addition to getting rid of the refguide_check.py script which is great. Just a few small comments.

Comment threadpywt/data/_readers.py Outdated
Comment threadpywt/_extensions/_pywt.pyx Outdated
pip install -r util/readthedocs/requirements.txt
sphinx-build -b html -W --keep-going -d _build/doctrees . doc/source doc/build
cd ..
# XXX sphinx build is broken on CI

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is happening here? Debug left-over, or is something actually broken right now? If so, I think three lines can be safely deleted. The separate CI job to build the docs with the ReadTheDocs integration is green.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The RTD job is not building the docs with -W, I guess: python -m sphinx -T -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html. Not that there are any warnings because we're already warning-free.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Indeed, this sphinx-build incantation did not previously run on CI because REFGUIDE-CHECK: [0], and was always broken. There's a standing offer to fix this in a follow-up though :-). #747 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay thanks, let's do that - offer is appreciated:)

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM now and CI is happy, so in it goes. Thanks @ev-br! And thanks @agriyakhetarpal for the review and help.

@rgommers
rgommers merged commit f2b3d58 into PyWavelets:mainJun 25, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ev-br@agriyakhetarpal@rgommers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Use scipy-doctest instead of refguide-check - #747

Merged
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs
Jun 25, 2024
Merged

Use scipy-doctest instead of refguide-check#747
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs

Conversation

@ev-br

@ev-brev-br commented Jun 2, 2024

Copy link
Copy Markdown
Contributor

SciPy has recently refactored its refguide-check utility into a separate pip-installable package and added integration of the modified doctesting with pytest. This PR switches the refguide-check run (which uses the utility vendored from scipy) to this refactored way.

There are several minor tweaks to docstrings --- I'm actually not sure all the affected docstrings were checked previously (but they are now):

  • Explicit $ doctest: comments are no longer needed
  • A refactor of the ContinuousWavelet example is to avoid a DeprecationWarning from matplotlib 3.7
  • Some whitespace tweaks where the printing is non-standard enough for the tool to give up and fall back to the (whitespace-sensitive) vanilla doctest checker.

The pytest incantations to run doctesting are explicit on the CI --- in SciPy, these are hidden behind the dev.py smoke-docs interface.

ev-br added 3 commits June 2, 2024 18:58
$ pytest --doctest-modules --pyargs pywt -v --doctest-collect=api
...
pywt/__init__.py::pywt.ContinuousWavelet.wavefun PASSED [ 3%]
pywt/__init__.py::pywt.Modes PASSED [ 6%]
pywt/__init__.py::pywt.Wavelet.wavefun PASSED [ 9%]
pywt/__init__.py::pywt.array_to_coeffs PASSED [ 12%]
pywt/__init__.py::pywt.coeffs_to_array PASSED [ 16%]
pywt/__init__.py::pywt.cwt PASSED [ 19%]
pywt/__init__.py::pywt.dwt PASSED [ 22%]
pywt/__init__.py::pywt.dwt2 PASSED [ 25%]
pywt/__init__.py::pywt.dwt_max_level PASSED [ 29%]
pywt/__init__.py::pywt.dwtn_max_level PASSED [ 32%]
pywt/__init__.py::pywt.families PASSED [ 35%]
pywt/__init__.py::pywt.fswavedecn PASSED [ 38%]
pywt/__init__.py::pywt.idwt PASSED [ 41%]
pywt/__init__.py::pywt.idwt2 PASSED [ 45%]
pywt/__init__.py::pywt.integrate_wavelet PASSED [ 48%]
pywt/__init__.py::pywt.iswt PASSED [ 51%]
pywt/__init__.py::pywt.iswt2 PASSED [ 54%]
pywt/__init__.py::pywt.iswtn PASSED [ 58%]
pywt/__init__.py::pywt.ravel_coeffs PASSED [ 61%]
pywt/__init__.py::pywt.threshold PASSED [ 64%]
pywt/__init__.py::pywt.unravel_coeffs PASSED [ 67%]
pywt/__init__.py::pywt.upcoef PASSED [ 70%]
pywt/__init__.py::pywt.wavedec PASSED [ 74%]
pywt/__init__.py::pywt.wavedec2 PASSED [ 77%]
pywt/__init__.py::pywt.wavedecn PASSED [ 80%]
pywt/__init__.py::pywt.wavedecn_shapes PASSED [ 83%]
pywt/__init__.py::pywt.wavedecn_size PASSED [ 87%]
pywt/__init__.py::pywt.wavelist PASSED [ 90%]
pywt/__init__.py::pywt.waverec PASSED [ 93%]
pywt/__init__.py::pywt.waverec2 PASSED [ 96%]
pywt/__init__.py::pywt.waverecn PASSED [100%]
================================== 31 passed in 0.80s =====================
Makes this green:
$ pytest --doctest-glob=*rst doc/source/regression/ -vs
@ev-brev-br changed the title Use Use scipy-doctest instead of refguide-checkJun 2, 2024

@agriyakhetarpalagriyakhetarpal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing this, @ev-br! All of the changes look good to me. I would suggest leaving out the files under doc/source/regression/, though – they are being converted to Markdown-style notebooks in gh-741, which means that the changes to them won't be needed since MyST-NB will execute them. That PR is close to getting merged and should be done by today or so.

ev-br added 2 commits June 3, 2024 18:15
The script can do two things:
- run modified doctests; this is done via smoke-docs / scipy-doctests now
- check __all__ lists vs refguide entries; this was already disabled, on CI at least
@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @agriyakhetarpal . Rolled back all changes to *rst files, removed the corresponding stanza from the CI run, and removed the refguide-check utility as "not used anymore". Would be nice if you could trigger the CI run for me, would you?

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

Thanks! I tried but it turns out I don't have enough permissions to trigger a CI run – tagging @rgommers who can do this for you.

@rgommers

Copy link
Copy Markdown
Member

triggered now - the joys of spammer-protection:)

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks Ralf! So the CI is green, thus the question is whether y'all want it :-). And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

@rgommers

Copy link
Copy Markdown
Member

Yes, and yes. thanks:)

And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

Do you want to include that here, or separately?

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

A follow-up PR would not require restarting the CI for me, so seems easier :-).

@rgommers

Copy link
Copy Markdown
Member

Did you actually notice the doctesting happening in CI? I only see REFGUIDE_CHECK: [0] in test.yml.

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Indeed! So it was never actually running on CI, huh :-). Pushed a commit to switch it always on. Depending on how you want to play this, can iterate the CI in follow-ups or here. (If the latter, a new commit asks for one more confirmation I'm not a crypto miner)

@rgommers

Copy link
Copy Markdown
Member
ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'util/readthedocs/requirements.txt'

If it becomes a pain for CI not to run, maybe submit a trivial typo/change PR that can be merged straight away?

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

With the latest failure, maybe we should do cd .. rather, much easier than fixing the paths in-place every time.

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@ev-br

ev-br commented Jun 4, 2024

Copy link
Copy Markdown
ContributorAuthor

Since sphinx on CI, which is the source of the failures, is not really related, maybe it's best to separate the fixes.

Comment thread.github/workflows/tests.yml Outdated
Comment thread.github/workflows/tests.yml Outdated
@ev-br

ev-br commented Jun 6, 2024

Copy link
Copy Markdown
ContributorAuthor

Okay, current status:

  • smoke-docs runs fine on CI
  • sphinx-build was disabled on CI, is still disabled on CI.
  • attempting to run sphinx-build locally shows a bunch of errors, so there's some structural problem with it.
  • However, docs build on readthedocs. Meaning, the problem is with the sphinx-build invocation which did not run on CI previously.

So how about not fixing the sphinx-build check in this PR? If that's OK, the PR is ready from my side.

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

I'll hope to fix the sphinx-build checks in a separate PR after this – I would be fine with this getting merged.

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ev-br, this looks quite nice! Overall much cleaner docs, in addition to getting rid of the refguide_check.py script which is great. Just a few small comments.

Comment threadpywt/data/_readers.py Outdated
Comment threadpywt/_extensions/_pywt.pyx Outdated
pip install -r util/readthedocs/requirements.txt
sphinx-build -b html -W --keep-going -d _build/doctrees . doc/source doc/build
cd ..
# XXX sphinx build is broken on CI

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is happening here? Debug left-over, or is something actually broken right now? If so, I think three lines can be safely deleted. The separate CI job to build the docs with the ReadTheDocs integration is green.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The RTD job is not building the docs with -W, I guess: python -m sphinx -T -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html. Not that there are any warnings because we're already warning-free.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Indeed, this sphinx-build incantation did not previously run on CI because REFGUIDE-CHECK: [0], and was always broken. There's a standing offer to fix this in a follow-up though :-). #747 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay thanks, let's do that - offer is appreciated:)

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM now and CI is happy, so in it goes. Thanks @ev-br! And thanks @agriyakhetarpal for the review and help.

@rgommers
rgommers merged commit f2b3d58 into PyWavelets:mainJun 25, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ev-br@agriyakhetarpal@rgommers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Use scipy-doctest instead of refguide-check - #747

Merged
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs
Jun 25, 2024
Merged

Use scipy-doctest instead of refguide-check#747
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs

Conversation

@ev-br

@ev-brev-br commented Jun 2, 2024

Copy link
Copy Markdown
Contributor

SciPy has recently refactored its refguide-check utility into a separate pip-installable package and added integration of the modified doctesting with pytest. This PR switches the refguide-check run (which uses the utility vendored from scipy) to this refactored way.

There are several minor tweaks to docstrings --- I'm actually not sure all the affected docstrings were checked previously (but they are now):

  • Explicit $ doctest: comments are no longer needed
  • A refactor of the ContinuousWavelet example is to avoid a DeprecationWarning from matplotlib 3.7
  • Some whitespace tweaks where the printing is non-standard enough for the tool to give up and fall back to the (whitespace-sensitive) vanilla doctest checker.

The pytest incantations to run doctesting are explicit on the CI --- in SciPy, these are hidden behind the dev.py smoke-docs interface.

ev-br added 3 commits June 2, 2024 18:58
$ pytest --doctest-modules --pyargs pywt -v --doctest-collect=api
...
pywt/__init__.py::pywt.ContinuousWavelet.wavefun PASSED [ 3%]
pywt/__init__.py::pywt.Modes PASSED [ 6%]
pywt/__init__.py::pywt.Wavelet.wavefun PASSED [ 9%]
pywt/__init__.py::pywt.array_to_coeffs PASSED [ 12%]
pywt/__init__.py::pywt.coeffs_to_array PASSED [ 16%]
pywt/__init__.py::pywt.cwt PASSED [ 19%]
pywt/__init__.py::pywt.dwt PASSED [ 22%]
pywt/__init__.py::pywt.dwt2 PASSED [ 25%]
pywt/__init__.py::pywt.dwt_max_level PASSED [ 29%]
pywt/__init__.py::pywt.dwtn_max_level PASSED [ 32%]
pywt/__init__.py::pywt.families PASSED [ 35%]
pywt/__init__.py::pywt.fswavedecn PASSED [ 38%]
pywt/__init__.py::pywt.idwt PASSED [ 41%]
pywt/__init__.py::pywt.idwt2 PASSED [ 45%]
pywt/__init__.py::pywt.integrate_wavelet PASSED [ 48%]
pywt/__init__.py::pywt.iswt PASSED [ 51%]
pywt/__init__.py::pywt.iswt2 PASSED [ 54%]
pywt/__init__.py::pywt.iswtn PASSED [ 58%]
pywt/__init__.py::pywt.ravel_coeffs PASSED [ 61%]
pywt/__init__.py::pywt.threshold PASSED [ 64%]
pywt/__init__.py::pywt.unravel_coeffs PASSED [ 67%]
pywt/__init__.py::pywt.upcoef PASSED [ 70%]
pywt/__init__.py::pywt.wavedec PASSED [ 74%]
pywt/__init__.py::pywt.wavedec2 PASSED [ 77%]
pywt/__init__.py::pywt.wavedecn PASSED [ 80%]
pywt/__init__.py::pywt.wavedecn_shapes PASSED [ 83%]
pywt/__init__.py::pywt.wavedecn_size PASSED [ 87%]
pywt/__init__.py::pywt.wavelist PASSED [ 90%]
pywt/__init__.py::pywt.waverec PASSED [ 93%]
pywt/__init__.py::pywt.waverec2 PASSED [ 96%]
pywt/__init__.py::pywt.waverecn PASSED [100%]
================================== 31 passed in 0.80s =====================
Makes this green:
$ pytest --doctest-glob=*rst doc/source/regression/ -vs
@ev-brev-br changed the title Use Use scipy-doctest instead of refguide-checkJun 2, 2024

@agriyakhetarpalagriyakhetarpal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing this, @ev-br! All of the changes look good to me. I would suggest leaving out the files under doc/source/regression/, though – they are being converted to Markdown-style notebooks in gh-741, which means that the changes to them won't be needed since MyST-NB will execute them. That PR is close to getting merged and should be done by today or so.

ev-br added 2 commits June 3, 2024 18:15
The script can do two things:
- run modified doctests; this is done via smoke-docs / scipy-doctests now
- check __all__ lists vs refguide entries; this was already disabled, on CI at least
@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @agriyakhetarpal . Rolled back all changes to *rst files, removed the corresponding stanza from the CI run, and removed the refguide-check utility as "not used anymore". Would be nice if you could trigger the CI run for me, would you?

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

Thanks! I tried but it turns out I don't have enough permissions to trigger a CI run – tagging @rgommers who can do this for you.

@rgommers

Copy link
Copy Markdown
Member

triggered now - the joys of spammer-protection:)

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks Ralf! So the CI is green, thus the question is whether y'all want it :-). And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

@rgommers

Copy link
Copy Markdown
Member

Yes, and yes. thanks:)

And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

Do you want to include that here, or separately?

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

A follow-up PR would not require restarting the CI for me, so seems easier :-).

@rgommers

Copy link
Copy Markdown
Member

Did you actually notice the doctesting happening in CI? I only see REFGUIDE_CHECK: [0] in test.yml.

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Indeed! So it was never actually running on CI, huh :-). Pushed a commit to switch it always on. Depending on how you want to play this, can iterate the CI in follow-ups or here. (If the latter, a new commit asks for one more confirmation I'm not a crypto miner)

@rgommers

Copy link
Copy Markdown
Member
ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'util/readthedocs/requirements.txt'

If it becomes a pain for CI not to run, maybe submit a trivial typo/change PR that can be merged straight away?

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

With the latest failure, maybe we should do cd .. rather, much easier than fixing the paths in-place every time.

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@ev-br

ev-br commented Jun 4, 2024

Copy link
Copy Markdown
ContributorAuthor

Since sphinx on CI, which is the source of the failures, is not really related, maybe it's best to separate the fixes.

Comment thread.github/workflows/tests.yml Outdated
Comment thread.github/workflows/tests.yml Outdated
@ev-br

ev-br commented Jun 6, 2024

Copy link
Copy Markdown
ContributorAuthor

Okay, current status:

  • smoke-docs runs fine on CI
  • sphinx-build was disabled on CI, is still disabled on CI.
  • attempting to run sphinx-build locally shows a bunch of errors, so there's some structural problem with it.
  • However, docs build on readthedocs. Meaning, the problem is with the sphinx-build invocation which did not run on CI previously.

So how about not fixing the sphinx-build check in this PR? If that's OK, the PR is ready from my side.

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

I'll hope to fix the sphinx-build checks in a separate PR after this – I would be fine with this getting merged.

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ev-br, this looks quite nice! Overall much cleaner docs, in addition to getting rid of the refguide_check.py script which is great. Just a few small comments.

Comment threadpywt/data/_readers.py Outdated
Comment threadpywt/_extensions/_pywt.pyx Outdated
pip install -r util/readthedocs/requirements.txt
sphinx-build -b html -W --keep-going -d _build/doctrees . doc/source doc/build
cd ..
# XXX sphinx build is broken on CI

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is happening here? Debug left-over, or is something actually broken right now? If so, I think three lines can be safely deleted. The separate CI job to build the docs with the ReadTheDocs integration is green.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The RTD job is not building the docs with -W, I guess: python -m sphinx -T -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html. Not that there are any warnings because we're already warning-free.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Indeed, this sphinx-build incantation did not previously run on CI because REFGUIDE-CHECK: [0], and was always broken. There's a standing offer to fix this in a follow-up though :-). #747 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay thanks, let's do that - offer is appreciated:)

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM now and CI is happy, so in it goes. Thanks @ev-br! And thanks @agriyakhetarpal for the review and help.

@rgommers
rgommers merged commit f2b3d58 into PyWavelets:mainJun 25, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ev-br@agriyakhetarpal@rgommers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Use scipy-doctest instead of refguide-check - #747

Merged
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs
Jun 25, 2024
Merged

Use scipy-doctest instead of refguide-check#747
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs

Conversation

@ev-br

@ev-brev-br commented Jun 2, 2024

Copy link
Copy Markdown
Contributor

SciPy has recently refactored its refguide-check utility into a separate pip-installable package and added integration of the modified doctesting with pytest. This PR switches the refguide-check run (which uses the utility vendored from scipy) to this refactored way.

There are several minor tweaks to docstrings --- I'm actually not sure all the affected docstrings were checked previously (but they are now):

  • Explicit $ doctest: comments are no longer needed
  • A refactor of the ContinuousWavelet example is to avoid a DeprecationWarning from matplotlib 3.7
  • Some whitespace tweaks where the printing is non-standard enough for the tool to give up and fall back to the (whitespace-sensitive) vanilla doctest checker.

The pytest incantations to run doctesting are explicit on the CI --- in SciPy, these are hidden behind the dev.py smoke-docs interface.

ev-br added 3 commits June 2, 2024 18:58
$ pytest --doctest-modules --pyargs pywt -v --doctest-collect=api
...
pywt/__init__.py::pywt.ContinuousWavelet.wavefun PASSED [ 3%]
pywt/__init__.py::pywt.Modes PASSED [ 6%]
pywt/__init__.py::pywt.Wavelet.wavefun PASSED [ 9%]
pywt/__init__.py::pywt.array_to_coeffs PASSED [ 12%]
pywt/__init__.py::pywt.coeffs_to_array PASSED [ 16%]
pywt/__init__.py::pywt.cwt PASSED [ 19%]
pywt/__init__.py::pywt.dwt PASSED [ 22%]
pywt/__init__.py::pywt.dwt2 PASSED [ 25%]
pywt/__init__.py::pywt.dwt_max_level PASSED [ 29%]
pywt/__init__.py::pywt.dwtn_max_level PASSED [ 32%]
pywt/__init__.py::pywt.families PASSED [ 35%]
pywt/__init__.py::pywt.fswavedecn PASSED [ 38%]
pywt/__init__.py::pywt.idwt PASSED [ 41%]
pywt/__init__.py::pywt.idwt2 PASSED [ 45%]
pywt/__init__.py::pywt.integrate_wavelet PASSED [ 48%]
pywt/__init__.py::pywt.iswt PASSED [ 51%]
pywt/__init__.py::pywt.iswt2 PASSED [ 54%]
pywt/__init__.py::pywt.iswtn PASSED [ 58%]
pywt/__init__.py::pywt.ravel_coeffs PASSED [ 61%]
pywt/__init__.py::pywt.threshold PASSED [ 64%]
pywt/__init__.py::pywt.unravel_coeffs PASSED [ 67%]
pywt/__init__.py::pywt.upcoef PASSED [ 70%]
pywt/__init__.py::pywt.wavedec PASSED [ 74%]
pywt/__init__.py::pywt.wavedec2 PASSED [ 77%]
pywt/__init__.py::pywt.wavedecn PASSED [ 80%]
pywt/__init__.py::pywt.wavedecn_shapes PASSED [ 83%]
pywt/__init__.py::pywt.wavedecn_size PASSED [ 87%]
pywt/__init__.py::pywt.wavelist PASSED [ 90%]
pywt/__init__.py::pywt.waverec PASSED [ 93%]
pywt/__init__.py::pywt.waverec2 PASSED [ 96%]
pywt/__init__.py::pywt.waverecn PASSED [100%]
================================== 31 passed in 0.80s =====================
Makes this green:
$ pytest --doctest-glob=*rst doc/source/regression/ -vs
@ev-brev-br changed the title Use Use scipy-doctest instead of refguide-checkJun 2, 2024

@agriyakhetarpalagriyakhetarpal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing this, @ev-br! All of the changes look good to me. I would suggest leaving out the files under doc/source/regression/, though – they are being converted to Markdown-style notebooks in gh-741, which means that the changes to them won't be needed since MyST-NB will execute them. That PR is close to getting merged and should be done by today or so.

ev-br added 2 commits June 3, 2024 18:15
The script can do two things:
- run modified doctests; this is done via smoke-docs / scipy-doctests now
- check __all__ lists vs refguide entries; this was already disabled, on CI at least
@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @agriyakhetarpal . Rolled back all changes to *rst files, removed the corresponding stanza from the CI run, and removed the refguide-check utility as "not used anymore". Would be nice if you could trigger the CI run for me, would you?

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

Thanks! I tried but it turns out I don't have enough permissions to trigger a CI run – tagging @rgommers who can do this for you.

@rgommers

Copy link
Copy Markdown
Member

triggered now - the joys of spammer-protection:)

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks Ralf! So the CI is green, thus the question is whether y'all want it :-). And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

@rgommers

Copy link
Copy Markdown
Member

Yes, and yes. thanks:)

And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

Do you want to include that here, or separately?

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

A follow-up PR would not require restarting the CI for me, so seems easier :-).

@rgommers

Copy link
Copy Markdown
Member

Did you actually notice the doctesting happening in CI? I only see REFGUIDE_CHECK: [0] in test.yml.

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Indeed! So it was never actually running on CI, huh :-). Pushed a commit to switch it always on. Depending on how you want to play this, can iterate the CI in follow-ups or here. (If the latter, a new commit asks for one more confirmation I'm not a crypto miner)

@rgommers

Copy link
Copy Markdown
Member
ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'util/readthedocs/requirements.txt'

If it becomes a pain for CI not to run, maybe submit a trivial typo/change PR that can be merged straight away?

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

With the latest failure, maybe we should do cd .. rather, much easier than fixing the paths in-place every time.

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@ev-br

ev-br commented Jun 4, 2024

Copy link
Copy Markdown
ContributorAuthor

Since sphinx on CI, which is the source of the failures, is not really related, maybe it's best to separate the fixes.

Comment thread.github/workflows/tests.yml Outdated
Comment thread.github/workflows/tests.yml Outdated
@ev-br

ev-br commented Jun 6, 2024

Copy link
Copy Markdown
ContributorAuthor

Okay, current status:

  • smoke-docs runs fine on CI
  • sphinx-build was disabled on CI, is still disabled on CI.
  • attempting to run sphinx-build locally shows a bunch of errors, so there's some structural problem with it.
  • However, docs build on readthedocs. Meaning, the problem is with the sphinx-build invocation which did not run on CI previously.

So how about not fixing the sphinx-build check in this PR? If that's OK, the PR is ready from my side.

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

I'll hope to fix the sphinx-build checks in a separate PR after this – I would be fine with this getting merged.

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ev-br, this looks quite nice! Overall much cleaner docs, in addition to getting rid of the refguide_check.py script which is great. Just a few small comments.

Comment threadpywt/data/_readers.py Outdated
Comment threadpywt/_extensions/_pywt.pyx Outdated
pip install -r util/readthedocs/requirements.txt
sphinx-build -b html -W --keep-going -d _build/doctrees . doc/source doc/build
cd ..
# XXX sphinx build is broken on CI

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is happening here? Debug left-over, or is something actually broken right now? If so, I think three lines can be safely deleted. The separate CI job to build the docs with the ReadTheDocs integration is green.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The RTD job is not building the docs with -W, I guess: python -m sphinx -T -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html. Not that there are any warnings because we're already warning-free.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Indeed, this sphinx-build incantation did not previously run on CI because REFGUIDE-CHECK: [0], and was always broken. There's a standing offer to fix this in a follow-up though :-). #747 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay thanks, let's do that - offer is appreciated:)

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM now and CI is happy, so in it goes. Thanks @ev-br! And thanks @agriyakhetarpal for the review and help.

@rgommers
rgommers merged commit f2b3d58 into PyWavelets:mainJun 25, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ev-br@agriyakhetarpal@rgommers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Use scipy-doctest instead of refguide-check - #747

Merged
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs
Jun 25, 2024
Merged

Use scipy-doctest instead of refguide-check#747
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs

Conversation

@ev-br

@ev-brev-br commented Jun 2, 2024

Copy link
Copy Markdown
Contributor

SciPy has recently refactored its refguide-check utility into a separate pip-installable package and added integration of the modified doctesting with pytest. This PR switches the refguide-check run (which uses the utility vendored from scipy) to this refactored way.

There are several minor tweaks to docstrings --- I'm actually not sure all the affected docstrings were checked previously (but they are now):

  • Explicit $ doctest: comments are no longer needed
  • A refactor of the ContinuousWavelet example is to avoid a DeprecationWarning from matplotlib 3.7
  • Some whitespace tweaks where the printing is non-standard enough for the tool to give up and fall back to the (whitespace-sensitive) vanilla doctest checker.

The pytest incantations to run doctesting are explicit on the CI --- in SciPy, these are hidden behind the dev.py smoke-docs interface.

ev-br added 3 commits June 2, 2024 18:58
$ pytest --doctest-modules --pyargs pywt -v --doctest-collect=api
...
pywt/__init__.py::pywt.ContinuousWavelet.wavefun PASSED [ 3%]
pywt/__init__.py::pywt.Modes PASSED [ 6%]
pywt/__init__.py::pywt.Wavelet.wavefun PASSED [ 9%]
pywt/__init__.py::pywt.array_to_coeffs PASSED [ 12%]
pywt/__init__.py::pywt.coeffs_to_array PASSED [ 16%]
pywt/__init__.py::pywt.cwt PASSED [ 19%]
pywt/__init__.py::pywt.dwt PASSED [ 22%]
pywt/__init__.py::pywt.dwt2 PASSED [ 25%]
pywt/__init__.py::pywt.dwt_max_level PASSED [ 29%]
pywt/__init__.py::pywt.dwtn_max_level PASSED [ 32%]
pywt/__init__.py::pywt.families PASSED [ 35%]
pywt/__init__.py::pywt.fswavedecn PASSED [ 38%]
pywt/__init__.py::pywt.idwt PASSED [ 41%]
pywt/__init__.py::pywt.idwt2 PASSED [ 45%]
pywt/__init__.py::pywt.integrate_wavelet PASSED [ 48%]
pywt/__init__.py::pywt.iswt PASSED [ 51%]
pywt/__init__.py::pywt.iswt2 PASSED [ 54%]
pywt/__init__.py::pywt.iswtn PASSED [ 58%]
pywt/__init__.py::pywt.ravel_coeffs PASSED [ 61%]
pywt/__init__.py::pywt.threshold PASSED [ 64%]
pywt/__init__.py::pywt.unravel_coeffs PASSED [ 67%]
pywt/__init__.py::pywt.upcoef PASSED [ 70%]
pywt/__init__.py::pywt.wavedec PASSED [ 74%]
pywt/__init__.py::pywt.wavedec2 PASSED [ 77%]
pywt/__init__.py::pywt.wavedecn PASSED [ 80%]
pywt/__init__.py::pywt.wavedecn_shapes PASSED [ 83%]
pywt/__init__.py::pywt.wavedecn_size PASSED [ 87%]
pywt/__init__.py::pywt.wavelist PASSED [ 90%]
pywt/__init__.py::pywt.waverec PASSED [ 93%]
pywt/__init__.py::pywt.waverec2 PASSED [ 96%]
pywt/__init__.py::pywt.waverecn PASSED [100%]
================================== 31 passed in 0.80s =====================
Makes this green:
$ pytest --doctest-glob=*rst doc/source/regression/ -vs
@ev-brev-br changed the title Use Use scipy-doctest instead of refguide-checkJun 2, 2024

@agriyakhetarpalagriyakhetarpal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing this, @ev-br! All of the changes look good to me. I would suggest leaving out the files under doc/source/regression/, though – they are being converted to Markdown-style notebooks in gh-741, which means that the changes to them won't be needed since MyST-NB will execute them. That PR is close to getting merged and should be done by today or so.

ev-br added 2 commits June 3, 2024 18:15
The script can do two things:
- run modified doctests; this is done via smoke-docs / scipy-doctests now
- check __all__ lists vs refguide entries; this was already disabled, on CI at least
@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @agriyakhetarpal . Rolled back all changes to *rst files, removed the corresponding stanza from the CI run, and removed the refguide-check utility as "not used anymore". Would be nice if you could trigger the CI run for me, would you?

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

Thanks! I tried but it turns out I don't have enough permissions to trigger a CI run – tagging @rgommers who can do this for you.

@rgommers

Copy link
Copy Markdown
Member

triggered now - the joys of spammer-protection:)

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks Ralf! So the CI is green, thus the question is whether y'all want it :-). And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

@rgommers

Copy link
Copy Markdown
Member

Yes, and yes. thanks:)

And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

Do you want to include that here, or separately?

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

A follow-up PR would not require restarting the CI for me, so seems easier :-).

@rgommers

Copy link
Copy Markdown
Member

Did you actually notice the doctesting happening in CI? I only see REFGUIDE_CHECK: [0] in test.yml.

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Indeed! So it was never actually running on CI, huh :-). Pushed a commit to switch it always on. Depending on how you want to play this, can iterate the CI in follow-ups or here. (If the latter, a new commit asks for one more confirmation I'm not a crypto miner)

@rgommers

Copy link
Copy Markdown
Member
ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'util/readthedocs/requirements.txt'

If it becomes a pain for CI not to run, maybe submit a trivial typo/change PR that can be merged straight away?

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

With the latest failure, maybe we should do cd .. rather, much easier than fixing the paths in-place every time.

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@ev-br

ev-br commented Jun 4, 2024

Copy link
Copy Markdown
ContributorAuthor

Since sphinx on CI, which is the source of the failures, is not really related, maybe it's best to separate the fixes.

Comment thread.github/workflows/tests.yml Outdated
Comment thread.github/workflows/tests.yml Outdated
@ev-br

ev-br commented Jun 6, 2024

Copy link
Copy Markdown
ContributorAuthor

Okay, current status:

  • smoke-docs runs fine on CI
  • sphinx-build was disabled on CI, is still disabled on CI.
  • attempting to run sphinx-build locally shows a bunch of errors, so there's some structural problem with it.
  • However, docs build on readthedocs. Meaning, the problem is with the sphinx-build invocation which did not run on CI previously.

So how about not fixing the sphinx-build check in this PR? If that's OK, the PR is ready from my side.

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

I'll hope to fix the sphinx-build checks in a separate PR after this – I would be fine with this getting merged.

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ev-br, this looks quite nice! Overall much cleaner docs, in addition to getting rid of the refguide_check.py script which is great. Just a few small comments.

Comment threadpywt/data/_readers.py Outdated
Comment threadpywt/_extensions/_pywt.pyx Outdated
pip install -r util/readthedocs/requirements.txt
sphinx-build -b html -W --keep-going -d _build/doctrees . doc/source doc/build
cd ..
# XXX sphinx build is broken on CI

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is happening here? Debug left-over, or is something actually broken right now? If so, I think three lines can be safely deleted. The separate CI job to build the docs with the ReadTheDocs integration is green.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The RTD job is not building the docs with -W, I guess: python -m sphinx -T -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html. Not that there are any warnings because we're already warning-free.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Indeed, this sphinx-build incantation did not previously run on CI because REFGUIDE-CHECK: [0], and was always broken. There's a standing offer to fix this in a follow-up though :-). #747 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay thanks, let's do that - offer is appreciated:)

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM now and CI is happy, so in it goes. Thanks @ev-br! And thanks @agriyakhetarpal for the review and help.

@rgommers
rgommers merged commit f2b3d58 into PyWavelets:mainJun 25, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ev-br@agriyakhetarpal@rgommers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Use scipy-doctest instead of refguide-check - #747

Merged
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs
Jun 25, 2024
Merged

Use scipy-doctest instead of refguide-check#747
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs

Conversation

@ev-br

@ev-brev-br commented Jun 2, 2024

Copy link
Copy Markdown
Contributor

SciPy has recently refactored its refguide-check utility into a separate pip-installable package and added integration of the modified doctesting with pytest. This PR switches the refguide-check run (which uses the utility vendored from scipy) to this refactored way.

There are several minor tweaks to docstrings --- I'm actually not sure all the affected docstrings were checked previously (but they are now):

  • Explicit $ doctest: comments are no longer needed
  • A refactor of the ContinuousWavelet example is to avoid a DeprecationWarning from matplotlib 3.7
  • Some whitespace tweaks where the printing is non-standard enough for the tool to give up and fall back to the (whitespace-sensitive) vanilla doctest checker.

The pytest incantations to run doctesting are explicit on the CI --- in SciPy, these are hidden behind the dev.py smoke-docs interface.

ev-br added 3 commits June 2, 2024 18:58
$ pytest --doctest-modules --pyargs pywt -v --doctest-collect=api
...
pywt/__init__.py::pywt.ContinuousWavelet.wavefun PASSED [ 3%]
pywt/__init__.py::pywt.Modes PASSED [ 6%]
pywt/__init__.py::pywt.Wavelet.wavefun PASSED [ 9%]
pywt/__init__.py::pywt.array_to_coeffs PASSED [ 12%]
pywt/__init__.py::pywt.coeffs_to_array PASSED [ 16%]
pywt/__init__.py::pywt.cwt PASSED [ 19%]
pywt/__init__.py::pywt.dwt PASSED [ 22%]
pywt/__init__.py::pywt.dwt2 PASSED [ 25%]
pywt/__init__.py::pywt.dwt_max_level PASSED [ 29%]
pywt/__init__.py::pywt.dwtn_max_level PASSED [ 32%]
pywt/__init__.py::pywt.families PASSED [ 35%]
pywt/__init__.py::pywt.fswavedecn PASSED [ 38%]
pywt/__init__.py::pywt.idwt PASSED [ 41%]
pywt/__init__.py::pywt.idwt2 PASSED [ 45%]
pywt/__init__.py::pywt.integrate_wavelet PASSED [ 48%]
pywt/__init__.py::pywt.iswt PASSED [ 51%]
pywt/__init__.py::pywt.iswt2 PASSED [ 54%]
pywt/__init__.py::pywt.iswtn PASSED [ 58%]
pywt/__init__.py::pywt.ravel_coeffs PASSED [ 61%]
pywt/__init__.py::pywt.threshold PASSED [ 64%]
pywt/__init__.py::pywt.unravel_coeffs PASSED [ 67%]
pywt/__init__.py::pywt.upcoef PASSED [ 70%]
pywt/__init__.py::pywt.wavedec PASSED [ 74%]
pywt/__init__.py::pywt.wavedec2 PASSED [ 77%]
pywt/__init__.py::pywt.wavedecn PASSED [ 80%]
pywt/__init__.py::pywt.wavedecn_shapes PASSED [ 83%]
pywt/__init__.py::pywt.wavedecn_size PASSED [ 87%]
pywt/__init__.py::pywt.wavelist PASSED [ 90%]
pywt/__init__.py::pywt.waverec PASSED [ 93%]
pywt/__init__.py::pywt.waverec2 PASSED [ 96%]
pywt/__init__.py::pywt.waverecn PASSED [100%]
================================== 31 passed in 0.80s =====================
Makes this green:
$ pytest --doctest-glob=*rst doc/source/regression/ -vs
@ev-brev-br changed the title Use Use scipy-doctest instead of refguide-checkJun 2, 2024

@agriyakhetarpalagriyakhetarpal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing this, @ev-br! All of the changes look good to me. I would suggest leaving out the files under doc/source/regression/, though – they are being converted to Markdown-style notebooks in gh-741, which means that the changes to them won't be needed since MyST-NB will execute them. That PR is close to getting merged and should be done by today or so.

ev-br added 2 commits June 3, 2024 18:15
The script can do two things:
- run modified doctests; this is done via smoke-docs / scipy-doctests now
- check __all__ lists vs refguide entries; this was already disabled, on CI at least
@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @agriyakhetarpal . Rolled back all changes to *rst files, removed the corresponding stanza from the CI run, and removed the refguide-check utility as "not used anymore". Would be nice if you could trigger the CI run for me, would you?

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

Thanks! I tried but it turns out I don't have enough permissions to trigger a CI run – tagging @rgommers who can do this for you.

@rgommers

Copy link
Copy Markdown
Member

triggered now - the joys of spammer-protection:)

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks Ralf! So the CI is green, thus the question is whether y'all want it :-). And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

@rgommers

Copy link
Copy Markdown
Member

Yes, and yes. thanks:)

And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

Do you want to include that here, or separately?

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

A follow-up PR would not require restarting the CI for me, so seems easier :-).

@rgommers

Copy link
Copy Markdown
Member

Did you actually notice the doctesting happening in CI? I only see REFGUIDE_CHECK: [0] in test.yml.

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Indeed! So it was never actually running on CI, huh :-). Pushed a commit to switch it always on. Depending on how you want to play this, can iterate the CI in follow-ups or here. (If the latter, a new commit asks for one more confirmation I'm not a crypto miner)

@rgommers

Copy link
Copy Markdown
Member
ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'util/readthedocs/requirements.txt'

If it becomes a pain for CI not to run, maybe submit a trivial typo/change PR that can be merged straight away?

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

With the latest failure, maybe we should do cd .. rather, much easier than fixing the paths in-place every time.

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@ev-br

ev-br commented Jun 4, 2024

Copy link
Copy Markdown
ContributorAuthor

Since sphinx on CI, which is the source of the failures, is not really related, maybe it's best to separate the fixes.

Comment thread.github/workflows/tests.yml Outdated
Comment thread.github/workflows/tests.yml Outdated
@ev-br

ev-br commented Jun 6, 2024

Copy link
Copy Markdown
ContributorAuthor

Okay, current status:

  • smoke-docs runs fine on CI
  • sphinx-build was disabled on CI, is still disabled on CI.
  • attempting to run sphinx-build locally shows a bunch of errors, so there's some structural problem with it.
  • However, docs build on readthedocs. Meaning, the problem is with the sphinx-build invocation which did not run on CI previously.

So how about not fixing the sphinx-build check in this PR? If that's OK, the PR is ready from my side.

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

I'll hope to fix the sphinx-build checks in a separate PR after this – I would be fine with this getting merged.

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ev-br, this looks quite nice! Overall much cleaner docs, in addition to getting rid of the refguide_check.py script which is great. Just a few small comments.

Comment threadpywt/data/_readers.py Outdated
Comment threadpywt/_extensions/_pywt.pyx Outdated
pip install -r util/readthedocs/requirements.txt
sphinx-build -b html -W --keep-going -d _build/doctrees . doc/source doc/build
cd ..
# XXX sphinx build is broken on CI

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is happening here? Debug left-over, or is something actually broken right now? If so, I think three lines can be safely deleted. The separate CI job to build the docs with the ReadTheDocs integration is green.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The RTD job is not building the docs with -W, I guess: python -m sphinx -T -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html. Not that there are any warnings because we're already warning-free.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Indeed, this sphinx-build incantation did not previously run on CI because REFGUIDE-CHECK: [0], and was always broken. There's a standing offer to fix this in a follow-up though :-). #747 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay thanks, let's do that - offer is appreciated:)

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM now and CI is happy, so in it goes. Thanks @ev-br! And thanks @agriyakhetarpal for the review and help.

@rgommers
rgommers merged commit f2b3d58 into PyWavelets:mainJun 25, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ev-br@agriyakhetarpal@rgommers
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Use scipy-doctest instead of refguide-check - #747

Merged
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs
Jun 25, 2024
Merged

Use scipy-doctest instead of refguide-check#747
rgommers merged 13 commits into
PyWavelets:mainfrom
ev-br:smoke-docs

Conversation

@ev-br

@ev-brev-br commented Jun 2, 2024

Copy link
Copy Markdown
Contributor

SciPy has recently refactored its refguide-check utility into a separate pip-installable package and added integration of the modified doctesting with pytest. This PR switches the refguide-check run (which uses the utility vendored from scipy) to this refactored way.

There are several minor tweaks to docstrings --- I'm actually not sure all the affected docstrings were checked previously (but they are now):

  • Explicit $ doctest: comments are no longer needed
  • A refactor of the ContinuousWavelet example is to avoid a DeprecationWarning from matplotlib 3.7
  • Some whitespace tweaks where the printing is non-standard enough for the tool to give up and fall back to the (whitespace-sensitive) vanilla doctest checker.

The pytest incantations to run doctesting are explicit on the CI --- in SciPy, these are hidden behind the dev.py smoke-docs interface.

ev-br added 3 commits June 2, 2024 18:58
$ pytest --doctest-modules --pyargs pywt -v --doctest-collect=api
...
pywt/__init__.py::pywt.ContinuousWavelet.wavefun PASSED [ 3%]
pywt/__init__.py::pywt.Modes PASSED [ 6%]
pywt/__init__.py::pywt.Wavelet.wavefun PASSED [ 9%]
pywt/__init__.py::pywt.array_to_coeffs PASSED [ 12%]
pywt/__init__.py::pywt.coeffs_to_array PASSED [ 16%]
pywt/__init__.py::pywt.cwt PASSED [ 19%]
pywt/__init__.py::pywt.dwt PASSED [ 22%]
pywt/__init__.py::pywt.dwt2 PASSED [ 25%]
pywt/__init__.py::pywt.dwt_max_level PASSED [ 29%]
pywt/__init__.py::pywt.dwtn_max_level PASSED [ 32%]
pywt/__init__.py::pywt.families PASSED [ 35%]
pywt/__init__.py::pywt.fswavedecn PASSED [ 38%]
pywt/__init__.py::pywt.idwt PASSED [ 41%]
pywt/__init__.py::pywt.idwt2 PASSED [ 45%]
pywt/__init__.py::pywt.integrate_wavelet PASSED [ 48%]
pywt/__init__.py::pywt.iswt PASSED [ 51%]
pywt/__init__.py::pywt.iswt2 PASSED [ 54%]
pywt/__init__.py::pywt.iswtn PASSED [ 58%]
pywt/__init__.py::pywt.ravel_coeffs PASSED [ 61%]
pywt/__init__.py::pywt.threshold PASSED [ 64%]
pywt/__init__.py::pywt.unravel_coeffs PASSED [ 67%]
pywt/__init__.py::pywt.upcoef PASSED [ 70%]
pywt/__init__.py::pywt.wavedec PASSED [ 74%]
pywt/__init__.py::pywt.wavedec2 PASSED [ 77%]
pywt/__init__.py::pywt.wavedecn PASSED [ 80%]
pywt/__init__.py::pywt.wavedecn_shapes PASSED [ 83%]
pywt/__init__.py::pywt.wavedecn_size PASSED [ 87%]
pywt/__init__.py::pywt.wavelist PASSED [ 90%]
pywt/__init__.py::pywt.waverec PASSED [ 93%]
pywt/__init__.py::pywt.waverec2 PASSED [ 96%]
pywt/__init__.py::pywt.waverecn PASSED [100%]
================================== 31 passed in 0.80s =====================
Makes this green:
$ pytest --doctest-glob=*rst doc/source/regression/ -vs
@ev-brev-br changed the title Use Use scipy-doctest instead of refguide-checkJun 2, 2024

@agriyakhetarpalagriyakhetarpal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for doing this, @ev-br! All of the changes look good to me. I would suggest leaving out the files under doc/source/regression/, though – they are being converted to Markdown-style notebooks in gh-741, which means that the changes to them won't be needed since MyST-NB will execute them. That PR is close to getting merged and should be done by today or so.

ev-br added 2 commits June 3, 2024 18:15
The script can do two things:
- run modified doctests; this is done via smoke-docs / scipy-doctests now
- check __all__ lists vs refguide entries; this was already disabled, on CI at least
@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @agriyakhetarpal . Rolled back all changes to *rst files, removed the corresponding stanza from the CI run, and removed the refguide-check utility as "not used anymore". Would be nice if you could trigger the CI run for me, would you?

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

Thanks! I tried but it turns out I don't have enough permissions to trigger a CI run – tagging @rgommers who can do this for you.

@rgommers

Copy link
Copy Markdown
Member

triggered now - the joys of spammer-protection:)

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks Ralf! So the CI is green, thus the question is whether y'all want it :-). And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

@rgommers

Copy link
Copy Markdown
Member

Yes, and yes. thanks:)

And if you do, whether you want a spin command to to mimic scipy's python dev.py smoke-docs.

Do you want to include that here, or separately?

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

A follow-up PR would not require restarting the CI for me, so seems easier :-).

@rgommers

Copy link
Copy Markdown
Member

Did you actually notice the doctesting happening in CI? I only see REFGUIDE_CHECK: [0] in test.yml.

@ev-br

ev-br commented Jun 3, 2024

Copy link
Copy Markdown
ContributorAuthor

Indeed! So it was never actually running on CI, huh :-). Pushed a commit to switch it always on. Depending on how you want to play this, can iterate the CI in follow-ups or here. (If the latter, a new commit asks for one more confirmation I'm not a crypto miner)

@rgommers

Copy link
Copy Markdown
Member
ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'util/readthedocs/requirements.txt'

If it becomes a pain for CI not to run, maybe submit a trivial typo/change PR that can be merged straight away?

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

With the latest failure, maybe we should do cd .. rather, much easier than fixing the paths in-place every time.

Comment thread.github/workflows/tests.yml Outdated
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@ev-br

ev-br commented Jun 4, 2024

Copy link
Copy Markdown
ContributorAuthor

Since sphinx on CI, which is the source of the failures, is not really related, maybe it's best to separate the fixes.

Comment thread.github/workflows/tests.yml Outdated
Comment thread.github/workflows/tests.yml Outdated
@ev-br

ev-br commented Jun 6, 2024

Copy link
Copy Markdown
ContributorAuthor

Okay, current status:

  • smoke-docs runs fine on CI
  • sphinx-build was disabled on CI, is still disabled on CI.
  • attempting to run sphinx-build locally shows a bunch of errors, so there's some structural problem with it.
  • However, docs build on readthedocs. Meaning, the problem is with the sphinx-build invocation which did not run on CI previously.

So how about not fixing the sphinx-build check in this PR? If that's OK, the PR is ready from my side.

@agriyakhetarpal

Copy link
Copy Markdown
Collaborator

I'll hope to fix the sphinx-build checks in a separate PR after this – I would be fine with this getting merged.

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @ev-br, this looks quite nice! Overall much cleaner docs, in addition to getting rid of the refguide_check.py script which is great. Just a few small comments.

Comment threadpywt/data/_readers.py Outdated
Comment threadpywt/_extensions/_pywt.pyx Outdated
pip install -r util/readthedocs/requirements.txt
sphinx-build -b html -W --keep-going -d _build/doctrees . doc/source doc/build
cd ..
# XXX sphinx build is broken on CI

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is happening here? Debug left-over, or is something actually broken right now? If so, I think three lines can be safely deleted. The separate CI job to build the docs with the ReadTheDocs integration is green.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The RTD job is not building the docs with -W, I guess: python -m sphinx -T -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html. Not that there are any warnings because we're already warning-free.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Indeed, this sphinx-build incantation did not previously run on CI because REFGUIDE-CHECK: [0], and was always broken. There's a standing offer to fix this in a follow-up though :-). #747 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay thanks, let's do that - offer is appreciated:)

@rgommersrgommers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM now and CI is happy, so in it goes. Thanks @ev-br! And thanks @agriyakhetarpal for the review and help.

@rgommers
rgommers merged commit f2b3d58 into PyWavelets:mainJun 25, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ev-br@agriyakhetarpal@rgommers