Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sphinx emits lots of warnings when building docs #911

Closed
kandersolar opened this issue Feb 25, 2020 · 10 comments · Fixed by #912
Closed

Sphinx emits lots of warnings when building docs #911

kandersolar opened this issue Feb 25, 2020 · 10 comments · Fixed by #912
Milestone

Comments

@kandersolar
Copy link
Member

Sphinx currently emits a few types of warnings when building the docs, none are really a big deal but it clutters the output and makes #909 less effective. I'll open a PR shortly to address:

Docutils warning

/usr/share/miniconda/envs/test_env/lib/python3.8/site-packages/sphinx/util/nodes.py:94: FutureWarning: 
   The iterable returned by Node.traverse()
   will become an iterator instead of a list in Docutils > 0.16.

I'm not 100% sure what this one is, but I assume it is related to the newest version of docutils deprecating something and sphinx hasn't caught up yet. example

I'll play around with pinning the docutils version and see if that fixes it.

Improper RST in gallery example

The I-V curve example generates WARNING: Title underline too short. because the page title got updated without extending the underline (my bad). Easy fix. link

First-line references

The first lines of docstrings get orphaned when they are copied into the API Reference docs page and Sphinx emits a bunch of warnings about it: example

There were a few votes in #909 to remove references from first lines of docstrings to prevent these warnings. If nobody objects, I'll move references into the second part of the docstring, for instance:

Current:

def nrel_earthsun_distance(time, how='numpy', delta_t=67.0, numthreads=4):
    """
    Calculates the distance from the earth to the sun using the
    NREL SPA algorithm described in [1]_.

    Parameters
    ----------
    time : pandas.DatetimeIndex

New:

def nrel_earthsun_distance(time, how='numpy', delta_t=67.0, numthreads=4):
    """
    Calculates the distance from the earth to the sun using the
    NREL SPA algorithm.

    The details of the NREL SPA algorithm are described in [1]_.

    Parameters
    ----------
    time : pandas.DatetimeIndex

This is related to #837 but that's a much bigger job and I'd rather not tackle it right now.

@mikofski
Copy link
Member

FYI: I fixed the 1st line reference issue for losses.py in #910

Calculates soiling ratio given particulate and rain data using the model
from Humboldt State University (HSU).
The HSU soiling model [1]_ returns the soiling ratio, a value between zero
and one which is equivalent to (1 - transmission loss). Therefore a soiling
ratio of 1.0 is equivalent to zero transmission loss.

Calculates fraction of energy lost due to soiling given rainfall data and
daily loss rate using the Kimber model.
Kimber soiling model [1]_ assumes soiling builds up at a daily rate unless
the daily rainfall is greater than a threshold. The model also assumes that
if daily rainfall has exceeded the threshold within a grace period, then
the ground is too damp to cause soiling build-up. The model also assumes
there is a maximum soiling build-up. Scheduled manual washes and rain
events are assumed to reset soiling to zero.

and I also noticed that they show up for gallery examples in the tooltip popup, so I removed it from the Kimber soiling example here too:

"""
Kimber Soiling Model
====================
Examples of soiling using the Kimber model.
"""
# %%
# This example shows basic usage of pvlib's Kimber Soiling model [1]_ with
# :py:meth:`pvlib.losses.soiling_kimber`.
#
# References
# ----------
# .. [1] "The Effect of Soiling on Large Grid-Connected Photovoltaic Systems
# in California and the Southwest Region of the United States," Adrianne
# Kimber, et al., IEEE 4th World Conference on Photovoltaic Energy
# Conference, 2006, :doi:`10.1109/WCPEC.2006.279690`
#
# The Kimber Soiling model assumes that soiling builds up at a constant rate
# until cleaned either manually or by rain. The rain must reach a threshold to
# clean the panels. When rains exceeds the threshold, it's assumed the earth is
# damp for a grace period before it begins to soil again. There is a maximum
# soiling build up that cannot be exceeded even if there's no rain or
# manual cleaning.
#
# Threshold
# ---------
# The example shown here demonstrates how the threshold affects soiling.
# Because soiling depends on rainfall, loading weather data is always the first
# step.

@kandersolar
Copy link
Member Author

Thanks for the heads up! I'll leave those alone/merge them in from master when #910 is merged.

@wholmgren
Copy link
Member

Thanks!

I assume it is related to the newest version of docutils deprecating something and sphinx hasn't caught up yet. example

Should we bump sphinx instead of (or in addition to) pinning docutils?

The first lines of docstrings get orphaned when they are copied into the API Reference docs page and Sphinx emits a bunch of warnings about it:

Is it the first line that matters or the first sentence or the first paragraph? #912 seems to take a few different approaches.

@mikofski
Copy link
Member

Should we bump sphinx instead of (or in addition to) pinning docutils?

+1

I just noticed yesterday that Sphinx is pinned at v1.8.5, which IMO is pretty old. I don't think this a usability concern because only for builders would care about this, so probably zero users would be affected by updating Sphinx to the newest version which is like 2.something I think.

I think it's okay to pin Sphinx and docutils after we confirm they are rendered to our satisfaction.

@kandersolar
Copy link
Member Author

Sorry for delayed reply. Pinning sphinx at 1.8.5 was decided in #833 (comment) because sphinx 2.0+ (at the time) rendered references poorly. I haven't tested it since then, so it could be that the most recent sphinx has improved reference formatting. I'll play around with that tonight.

Is it the first line that matters or the first sentence or the first paragraph? #912 seems to take a few different approaches.

At first I thought that it was the first paragraph that mattered, but I realized halfway through that autodoc is only pulling the first English sentence (or at least something that looks like the first sentence). So I think second sentence, first paragraph is fine for references.

@wholmgren
Copy link
Member

Thanks for the clarification. I guess the paragraph/sentence distinction wouldn't matter if we decided to follow pep 257.

@kandersolar
Copy link
Member Author

Running with the most recent sphinx (2.4.3) and docutils (0.16) still produces the reference format seen in #833. After trying it out for a bit, I think the CSS override necessary to get it looking nice again would be pretty unpleasant and I wouldn't want to maintain it. However, sphinx 2.0+ does allow use of the original HTML4 writer use in sphinx 1.8.5, which produces good output with the newest sphinx/docutils. The docutils warning is gone, but another has popped up:

/home/kevin/anaconda3/lib/python3.7/site-packages/sphinx_rtd_theme/search.html:20: RemovedInSphinx30Warning: To modify script_files in the theme is deprecated. Please insert a <script> tag directly in your theme instead.
  {{ super() }}

It's fixed as of readthedocs/sphinx_rtd_theme#728, but unfortunately there hasn't been a release since the merge (readthedocs/sphinx_rtd_theme#739) so isn't available on pypi. I think this warning doesn't break the build, even with -W, so the only downside is extra clutter in the output. So to summarize, these are the three options that give good output:

  • Option 1 (current): sphinx==1.8.5, docutils==0.16: docutils warning
  • Option 2 (Fix most sphinx warnings #912 as of 6098122): sphinx==1.8.5, docutils==0.15.2: no warning
  • Option 3: sphinx==2.4.3, docutils==0.16, html4_writer=True: non-breaking warning

I don't really have a preference for Option 2 vs 3. Curious what others think.

@mikofski
Copy link
Member

my vote: Option 2 (#912 as of 6098122): sphinx==1.8.5, docutils==0.15.2: no warning

@wholmgren
Copy link
Member

Yikes what a mess. Thanks for the research! I’m ok with either 2 or 3.

@kandersolar
Copy link
Member Author

I'd say let's do option 2 for now and revisit after sphinx_rtd_theme cuts a new release. From reading through their issue log, it seems like the next release is focused on improving support for sphinx 2+ anyway so it should be a good time to bump our sphinx version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants