pvlib-python icon indicating copy to clipboard operation
pvlib-python copied to clipboard

Port notebooks to a gallery

Open echedey-ls opened this issue 1 year ago • 5 comments

  • [x] Closes #120
  • [x] I am familiar with the contributing guidelines
  • ~[ ] Tests added~
  • [ ] Updates entries in docs/sphinx/source/reference for API changes.
  • [ ] Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • [x] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • [ ] Pull request is nearly complete and ready for detailed review.
  • [ ] Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Addresses #120.

I've made a new sphinx gallery out of the content of the .ipynb notebooks.

View documentation front page for this PR View tutorial gallery

Ported notebooks (checked ones can be reviewed)

Checklist of reviewed by me

  • [x] hyperlinks
  • [ ] remove "notebook" references (PENDING FOR ALL)
  • [x] function links
  • [x] check I did not leave anything commented because it doesn't work (ehem ehem I need help)
  • [x] check bullet lists are correctly rendered

Various points:

  • I am proposing another gallery; maybe we don't want that and just a folder inside the current examples one (I can open a PR so you can compare the rendering)
  • Maybe we don't even want a gallery, alternatives just don't come to mind
  • Most of the tutorials were translated with:
jupyter nbconvert --to script tracking.ipynb
black --line-length 79 tracking.py

then used a list of regexes to find and replace things

Details

Remove In [*]:
Find: # In\[[0-9]+\]:\n\n
Replace: # %%

Hyperlink to sections:
F: \[(.*)\]\(.*\)
R: `$1`_

Convert MD math to rST inline
F: \$(.*?)\$
R: :math:`$1`

Subsections
F: # #{2} (.*)\n
R: # %%\n# $1\n# -------------------------------------

Subsubsections:
F: # #{3} (.*)\n
R: # %%\n# $1\n# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Find *possible* functions that need linking: ``
finally, visually inspected where text and code blocks should go so that related explanations are grouped all together and make sense

Expect to still find typos regarding that.

Feel free to point missed things and so on.

echedey-ls avatar Jul 31 '23 16:07 echedey-ls

Can we have #1763 merged before this one? I've left some TODOs to remove redundant code.

Also, I've commented code at irradiance.py:614 (and at line 655 because of that) because it raises the following error:

Unexpected failing examples:
/home/docs/checkouts/readthedocs.org/user_builds/pvlib-python/checkouts/1818/docs/tutorials/irradiance.py failed leaving traceback:
Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/pvlib-python/checkouts/1818/docs/tutorials/irradiance.py", line 619, in <module>
    + F1c[ebin, 2] * z[ebin.index]
IndexError: index 8 is out of bounds for axis 0 with size 8

I expect the problem to be at the internal function called on line 617:

F1c, F2c = pvlib.irradiance._get_perez_coefficients(modelt)

View code.

@wholmgren can you have a look at it? TIA.

echedey-ls avatar Aug 03 '23 11:08 echedey-ls

@echedey-ls thanks for taking the initiative! That issue is 7 years old and the pvlib documentation has changed a lot since then. I am not sure that 1:1 ports into a new Tutorial section are the right approach at this time. It's worth revisiting #329 especially @kandersolar comment https://github.com/pvlib/pvlib-python/issues/329#issuecomment-1277966321 before going too far here. Unfortunately I no longer have the vision or time to do documentation well and would defer to other maintainers and community members.

wholmgren avatar Aug 04 '23 16:08 wholmgren

Thanks @wholmgren , I could figure out the issue with the broken tutorial, the error was on my side: just a commented out line.

I agree on the view about everything to be fair. However, I think it is worth to reuse the old tutorials (they still work well) and give them presence on the web page, so they are easier to spot for anyone so another source of knowledge for users that don't download the repo, and, what I think is the nicest point, they get tested with CI - better to integrate them today than in 7 years, luckily.

Btw I like taking the over-engineering path so we all see how another gallery could benefit us. Or if we prefer just a subsection in the examples gallery. Open to all alternatives, and desiring to improve current docs.

On the other hand, if we have a gallery or section of tutorials then that could ease the port of examples to tutorials (if they are more intended to do so) as stated in Kevin's comment in # 329, split long tutorials, or even make an effort and make new ones. Honestly, I believe this PR is on that path, or even if it wasn't, raising discussion for concrete next steps regarding old tutorials would be fine and I would be willing to help with that (if it is a reasonable amount of time, i.e. I can't create a whole new tutorial)

Unfortunately I no longer have the vision or time to do documentation well and would defer to other maintainers and community members.

Even if it was just a matter of willingness; we've got your back.

echedey-ls avatar Aug 05 '23 19:08 echedey-ls

I am also hesitant to directly dump the tutorial notebooks into a new Tutorials docs section. My concerns:

  1. There is certainly good content in them, but some of the explanation needs to be expanded and/or updated for today's pvlib. Some of the content might also not be within scope for pvlib's docs (looking mostly at the tracking notebook). I think a substantial cleanup would be necessary for the notebook contents to be satisfying additions to the sphinx docs.
  2. I'm not sure that a new Tutorials section is the best home for this content. It could be that a new section in the existing gallery would be better. Or new User's Guide pages. More thought is needed about the role that we want these notebooks to have in our docs, with #329 in mind. The reason I think we should be deliberate here is that, even though semver doesn't apply to documentation, I think it is still desirable to minimize unnecessary "churn" in the documentation, especially in regards to changing the URLs of pages. So just like we take care when choosing function names and what modules they live in so that we won't have to change them in the long run, I think we should be thinking ahead and trying to avoid setting ourselves up to have to move these pages somewhere else before long.

I think converting the ipynb files to rst is a useful start that will certainly be helpful when we start working on #329 in earnest. For now I don't think we're quite ready to proceed with adding this content to the sphinx documentation. I think first we need to do the documentation restructure, and then we can come back and add this content into the new structure.

For whenever we do go forward with this PR in some form, here are a few things I noticed:

  • this tutorial needs numba for the speed portion to be useful: https://pvlib-python--1818.org.readthedocs.build/en/1818/gallery_tutorials/solarposition.html
  • Math is using dollar signs instead of :math:: https://pvlib-python--1818.org.readthedocs.build/en/1818/gallery_tutorials/irradiance.html
  • Bad RST rendering: https://pvlib-python--1818.org.readthedocs.build/en/1818/gallery_tutorials/tracking.html
  • Bad math rendering: https://pvlib-python--1818.org.readthedocs.build/en/1818/gallery_tutorials/pvsystem.html

kandersolar avatar Aug 08 '23 15:08 kandersolar

Uhm, I see.

I'll stop working on this PR, feel free to close if you think so, but I'll keep the translated tutorials just in case. Maybe I do tweaks to some of them to leverage future work.

Thanks for the in-depth explanation.

echedey-ls avatar Aug 09 '23 13:08 echedey-ls