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

Port notebooks to a gallery #1818

Closed

Conversation

echedey-ls
Copy link
Contributor

@echedey-ls echedey-ls commented Jul 31, 2023

  • Closes Port notebook tutorials to rst with doctests #120
  • 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`).
  • 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

  • hyperlinks
  • remove "notebook" references (PENDING FOR ALL)
  • function links
  • check I did not leave anything commented because it doesn't work (ehem ehem I need help)
  • 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 echedey-ls changed the title WIP: Port notebooks to example gallery WIP: Port notebooks to a gallery Aug 1, 2023
@echedey-ls
Copy link
Contributor Author

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.

@wholmgren
Copy link
Member

@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 #329 (comment) 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.

@echedey-ls echedey-ls changed the title WIP: Port notebooks to a gallery Port notebooks to a gallery Aug 5, 2023
@echedey-ls
Copy link
Contributor Author

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.

@kandersolar
Copy link
Member

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 define/improve documentation structure #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:

@echedey-ls
Copy link
Contributor Author

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
Copy link
Contributor Author

Considering the current state of pvlib, I think it's safe to assume this tutorials will be deleted at some point in the future, so I'm closing this PR as unnecessary.

@echedey-ls echedey-ls closed this Sep 11, 2024
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.

Port notebook tutorials to rst with doctests
3 participants