Skip to content

Fix Doctests - #2499

Merged
scarlehoff merged 28 commits into
masterfrom
doctest
Sep 1, 2026
Merged

Fix Doctests#2499
scarlehoff merged 28 commits into
masterfrom
doctest

Conversation

@ecole41

@ecole41 ecole41 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

This PR addresses this Issue.

Many of the examples given in docstrings were out of date. The majority of these have been reformatted and now pass doctest. Where executing an example is unnecessary, it has been converted to a Python code block instead.

To Do:

  • Fix docstring examples in n3fit_data.py

@ecole41 ecole41 self-assigned this Jul 9, 2026
Comment thread validphys2/src/validphys/pseudodata.py Outdated
Comment thread validphys2/src/validphys/pdfbases.py Outdated
Comment thread validphys2/src/validphys/calcutils.py Outdated
@ecole41 ecole41 added the redo-regressions Recompute the regression data label Jul 22, 2026
@scarlehoff scarlehoff removed the redo-regressions Recompute the regression data label Aug 18, 2026
Comment thread validphys2/src/validphys/n3fit_data.py Outdated

# can't use collect here because integdatasets might not exist.
def integdatasets_fitting_integ_dict(integdatasets=None):
def integdatasets_fitting_integ_dict(integdatasets):

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.

The reason the regression testa are failing are these two lines.
I don't know whether you removed these =None or whether you simply were using a very old version of the code when you added your commits

@scarlehoff

Copy link
Copy Markdown
Member

You changed it for the integrability but not for the positivity.

I'm not sure whether you merged back some old branch.

I recommend you revert back your local version to this commit e489d6d

and update the code there.

@ecole41

ecole41 commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

@scarlehoff I have just reverted the positivity dictionary function back to the master version. Now the changes only affect the docstrings.

@scarlehoff

Copy link
Copy Markdown
Member

What command did you use to check they pass doctests? I tried with pytest --doctest-modules and got many "errors" like

>>> loss_h(chi2) == np.asarray(c * (chi2-chi2ref))
Expected:
    True
Got:
    np.True_

(which would be fixed by doing bool on the numpy side for instance)

Clearly, these are not errors in that they are both True (just a different kind of True) but I wonder whether I'm using the wrong doctest module to test.

(I wanted to ask you to add it to the github workflow)

@ecole41

ecole41 commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

What command did you use to check they pass doctests? I tried with pytest --doctest-modules and got many "errors" like

>>> loss_h(chi2) == np.asarray(c * (chi2-chi2ref))
Expected:
    True
Got:
    np.True_

(which would be fixed by doing bool on the numpy side for instance)

Clearly, these are not errors in that they are both True (just a different kind of True) but I wonder whether I'm using the wrong doctest module to test.

(I wanted to ask you to add it to the github workflow)

You are right, I hadn't yet added the changed to the n3fit module but it should be working now. I have fixed the docstrings which were out of date and converted the examples in the vpinterface.py file into python code blocks as these have items printed during running so it's tricky to get them working within a doctest.

I have been using pytest --doctest-modules --ignore=tests . within n3fit/src/n3fit and validphys2/src/validphys/

@scarlehoff

Copy link
Copy Markdown
Member

ah, okok, I thought perhaps there was a way to tell pytest to autoconvert the True(s), ignore the strings, etc.

Running it with:

pytest --doctest-modules n3fit/src/n3fit/ validphys2/src/validphys/ --ignore-glob=*test_* --ignore=validphys2/serverscripts

I'm still getting a few errors. Most of them again things like

144 Example
145 -------
146 >>> import numpy as np
147 >>> from n3fit.layers import losses
148 >>> pred = np.random.rand(1, 1, 5)
149 >>> alpha = 1e-7
150 >>> c = 1e8
151 >>> loss_f = losses.LossPositivity(c=c, alpha=alpha)
152 >>> np.isclose(loss_f(pred).numpy()[0], -5 * alpha)
Expected:
    True
Got:
    np.True_

so again, not really "errors" but doctest complains

Also, you probably didn't get this one (since you might have installed 3.1 already) but I think that Loader should be changed to FallbackLoader

089 Examples
090 --------
091 Compute the maximum difference across replicas between the u and ubar PDFs
092 (times x) for x=0.05 and both Q=10 and Q=100::
093
094     >>> from validphys.loader import Loader
095     >>> from validphys.gridvalues import grid_values
096     >>> import numpy as np
097     >>> gv = grid_values(Loader().check_pdf('NNPDF31_nnlo_as_0118'), [-1, 1], [0.5], [10, 100])
UNEXPECTED EXCEPTION: PDFNotFound('NNPDF31_nnlo_as_0118')

When you do that I'll add a new entry to all_tests_nnpdf.yml with

pytest --doctest-modules n3fit/src/n3fit/ validphys2/src/validphys/ --ignore-glob=*test_* --ignore=validphys2/serverscripts

(or if you want to try feel free, but perhaps ignore as well n3fit/src/n3fit/hyper_optimization/ there if it doesn't work ootb in your computer, you can probably copy run_package_tests, but just for python 3.13 and ubuntu should suffice )

@ecole41

ecole41 commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

I don't seem to get this error, they all pass for me. What version of numpy are you using?
Also, I will change that Loader

@scarlehoff

Copy link
Copy Markdown
Member

ah, that's interesting. Perhaps it's worth to try it directly in the CI then...

@scarlehoff scarlehoff 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, unless there's some specific reason to run the test only for labelled PRs I think it'd be better to do it at every push. It takes about 6 minutes so it is quite cheap.

Comment thread .github/workflows/doctests.yml Outdated

on:
pull_request:
types: [opened, synchronize, reopened, labeled]

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.

I would just do it on every push tbh.
Is there any reason to do it only on labelled PRs?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There was no particular reason other than it allowing us to have the flexibility to not run the test, but as it's fast then I can't see a reason why we wouldn't want to run it. I'll change this

@ecole41 ecole41 removed the doctest label Sep 1, 2026
@scarlehoff
scarlehoff merged commit 5f89dd0 into master Sep 1, 2026
14 of 15 checks passed
@scarlehoff
scarlehoff deleted the doctest branch September 1, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants