-
Notifications
You must be signed in to change notification settings - Fork 260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed documentation build error and warnings #1163
Conversation
Codecov ReportBase: 92.38% // Head: 92.38% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #1163 +/- ##
=======================================
Coverage 92.38% 92.38%
=======================================
Files 98 98
Lines 12264 12264
Branches 2520 2520
=======================================
Hits 11330 11330
Misses 613 613
Partials 321 321
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@effigies there are also suddenly doctest failures that weren't there before. Any idea what happened? |
I think there's something that happened with upgrading sphinx that changed the behavior of autosummary. I believe that the |
Are these really meant to be doctests? If so, shouldn't we either include the imports or add |
Yes, the idea is that all of the example code should run, so we know the documentation is valid. Could be that there was an implicit context in earlier sphinx that now needs to be managed directly. |
No problem. Do you think it would be better to import all the relevant objects in the global context or per test? E.g., in quaternions.py: >>> import numpy as np
>>> from nibabel.quaternions import quat2mat # Added
>>> q = [0, 1, 0, 0] # 180 degree rotation around axis 0
>>> M = quat2mat(q) # from this module
>>> vec = np.array([1, 2, 3]).reshape((3,1)) # column vector
>>> tvec = np.dot(M, vec) or add all the required imports in I'm leaning towards the first option. WDYT? |
Wait, so doctests in the current file don't have access to the variables in that file? That seems like a regression in sphinx, or possibly some option that was switched from defaulting true to false. |
Seems this is a known issue: sphinx-doc/sphinx#6590 |
From what I gather, it doesn't seem like there's an easy workaround, nor that this will be resolved any time soon. Any suggestions? |
Let's just import the current module. |
There's a |
# Autosummary always wants to use a `generated/` directory. | ||
# We generate with `make api-stamp` | ||
# This could change in the future | ||
autosummary_generate = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthew-brett Is the make api-stamp
replaceable with some invocation of autosummary
? I don't fully understand everything that's going on here.
I'm going to merge this. We can do additional fixups if you spot anything... |
This resolves #1162, however, there are still some documents not included in any toctree, which raises warnings.
Should any of these be removed? Or included in any existing / new section?