DOC, TST Update doctests ref guide by pdebuyl · Pull Request #15519 · numpy/numpy

pdebuyl · 2020-02-05T14:25:20Z

Mostly "mechanical" changes following the changes of representation, the print function, or missing definitions.

Remove the timezone in the output, which is deprecated since 1.11. Skip the output showing deliberately the old and deprecated version.

Fix missing np prefix. Use print function instead of the statement. Add seed to make output repeatable.

Print statements, changes in the array repr.

doc/source/reference/arrays.datetime.rst

eric-wieser · 2020-02-05T20:51:57Z

doc/source/reference/arrays.nditer.rst

    ...     print("%d <%d>" % (it[0], it.index), end=' ')
    ...     it.iternext()
    ...
-    0 <0> 1 <2> 2 <4> 3 <1> 4 <3> 5 <5>
+    0 <0> True
+    1 <2> True
+    2 <4> True
+    3 <1> True
+    4 <3> True
+    5 <5> False


This doesn't look right - what is printing the booleans?

(edit: xref #15519 (comment))

In a IPython terminal, I obtain the "old" result. When running the refguide_check.py tool the output is as in the commit.

In a "plain" Python REPL, the output contains the booleans. All lines that return a value seem to appear. This type of detail is hard to find in the Python docs, it seems that it is "assumed to be known"...

doc/source/reference/maskedarray.baseclass.rst

eric-wieser

Mostly looks good. I'd like:

@mattip's feedback on the use of doctest markers here
To understand the True / False printouts in the iteration examples

Co-Authored-By: Eric Wieser <[email protected]>

eric-wieser · 2020-02-05T21:11:43Z

doc/source/reference/arrays.nditer.rst

@@ -220,20 +220,37 @@ produce identical results to the ones in the previous section.
    ...     print("%d <%d>" % (it[0], it.index), end=' ')
    ...     it.iternext()


It's nasty, but this would make the output go away:

Suggested change

... it.iternext()

... it.iternext()

... None # needed to make the docs build properly :(

Why not v = it.iternext()

Well, the output that I put in the PR corresponds to the REPL output, that differs from the IPython output. It is a matter of choice between the two.

I think we could make both the repl and ipython have the same output if we change line 221 to be v = it.iternext() (and line 233 too)

I agree. However, it remains hacky: the example is modified for the purpose of a clean doctest but the modification does not reflect a programming necessity or a good practice. That being said, you are in a much better position to make the choice for this matter of style, let me know your decision.

I think it is easier to hide this detail in an unused value than to try to explain why the loop is printing booleans. Good practice would be to use the for loop anyway, I am not a big fan of looping over an iterator with while.

Perhaps is_not_finished = ... instead?

, I am not a big fan of looping over an iterator with while.

Even that would be a better example, since next is a lot more useful than our iternext.

So, for this PR, we keep the while and I add is_not_finished = ?

doc/source/reference/arrays.datetime.rst

mattip · 2020-02-06T15:05:56Z

Does doc/source/reference/arrays.datetime.rst pass cleanly for you? When I run it (after python -m pip install . on latest HEAD and a python 3.6 virtualenv), I see problems with DeprecationWarnings. It seems to be an issue with capturing the warnings when running the doctest. I think it would be better to leave that file out for this PR.

pdebuyl · 2020-02-06T15:37:43Z

Does doc/source/reference/arrays.datetime.rst pass cleanly for you? When I run it (after python -m pip install . on latest HEAD and a python 3.6 virtualenv), I see problems with DeprecationWarnings. It seems to be an issue with capturing the warnings when running the doctest. I think it would be better to leave that file out for this PR.

I also have the warning issue. The PR does not fix all errors, see #14970 (comment)

Regarding the warning, it seems that the doctest runner at

numpy/tools/refguide_check.py

Line 766 in dae4f67

def _run_doctests(tests, full_name, verbose, doctest_warnings):

tries to capture warnings but fails. So, in principle, this should work.

pdebuyl · 2020-02-06T15:38:58Z

PS: how do you run pep8 on the rst files?

mattip · 2020-02-06T16:03:35Z

So, in principle, this should work.

Right, so it failing to work is a separate issue. It would be much easier to review your changes if I could (1) run the tests on master, see that they fail, (2) run them on your branch see that they pass, (3) review the changes for anything that looks off. But since (2) fails it means I have to think harder about whether the changes are correct, which is why I would prefer putting off the changes on that file until my preferred workflow is possible.

mattip · 2020-02-06T16:07:10Z

PS: how do you run pep8 on the rst files?

Another reason to move to astropy's solution: we could file an issue there and maybe someone would add support :)

pdebuyl · 2020-02-06T20:52:56Z

@mattip I created a separate PR #15532 where all files commited pass the tests. I'll get back to the remaining failing files in the reference guide next week.

pdebuyl · 2020-02-07T09:26:18Z

I close following the merge of #15532 and will get back to it next week.

Should we discuss the strategy for doctesting the guide? At https://github.com/scipy-lectures/scipy-lecture-notes we run pytest on all rst files for instance. I see that astropy also developed a tool, I will give it a look.

eric-wieser · 2020-02-07T10:24:16Z

Are there parts of this that didn't make it into #15532? It might be useful to reopen and rebase this, so we can see what's still to handle - even if we consider it in a WIP state.

pdebuyl · 2020-02-07T10:45:41Z

For the files arrays.nditer.rst and arrays.datetime.rst this PR decreased the number of errors but did not suppress all of them.

For arrays.datetime.rst: the only remaining problem is that the test runner did not output the warning.

For arrays.nditer.rst there was the while loop output issue but there remained many other errors that I did not investigate yet.

eric-wieser · 2020-02-07T14:30:21Z

Was hoping that would recompute the diff, but it doesn't seem to have

pdebuyl added 5 commits February 5, 2020 14:58

update doctest output for datetime arrays

97f8602

Remove the timezone in the output, which is deprecated since 1.11. Skip the output showing deliberately the old and deprecated version.

update doctests, various small bugs

f7a52ac

Fix missing np prefix. Use print function instead of the statement. Add seed to make output repeatable.

update doctests for minor bugs

ec866b3

Print statements, changes in the array repr.

update doctests

d7b4308

fix doctest for missing variable output or definition

fe46037

pdebuyl requested a review from eric-wieser February 5, 2020 20:47