JaGeo / LobsterPy

Package to perform automatic bonding analysis with the program Lobster in the field of computational materials science and quantum chemistry
https://jageo.github.io/LobsterPy/
BSD 3-Clause "New" or "Revised" License
74 stars 27 forks source link

tutorial: replace os with pathlib.Path #231

Closed berquist closed 6 months ago

berquist commented 6 months ago

Part of https://github.com/openjournals/joss-reviews/issues/6286.

This also fixes an issue I had with the git sparse checkout command.

Some questions:

naik-aakash commented 6 months ago

Hi @berquist , Thank you for this PR and suggestions!

Some explanation on the choices I made for the tutorial

  1. There is not any specific need for sparse checkout, we just wanted users to have only data needed for running tutorial. I assumed users interested will download the notebook from the documentation website and won't be interested in having a local copy of the code base. We also provide the commands for it. So should not be much of a hassle I think. Maybe you have other thoughts on it. Do you think this is too advanced? Happy to listen to it more.
  2. Agreed, in the rendered docs, we hide the code blocks which set directories, this is because, when rendering the docs, we are actually executing the complete juypter notebooks real time using the files from repo. If we remove them our automatic docs generation will break. And also if we show them the directory structure could be bit more confusing for users and current way felt simpler to have it consistent with files users download.

We would be glad to have any kind of suggestions or changes to make it more accessible. 😄

JaGeo commented 6 months ago

@berquist Thank you for making those changes! And also for your comments.

@naik-aakash and I are, of course, happy to follow your suggestions if you think hiding the directories is problematic for using the tutorials or that using sparse checkout is too advanced. @naik-aakash laid out the reasoning from our side above. For example, we could add some more explanation for the git command that we are using and mention the alternative of downloading the whole repo.

berquist commented 6 months ago

I didn't think of the use case where one downloads only the notebook, which is entirely possible if someone has been following docs and installed LobsterPy from PyPI and not git. I won't touch those instructions then. The commands are advanced but they are short. It is probably not important to explain them since it may be distracting from the tutorial. Mentioning that this documentation page is a Jupyter Notebook that can be downloaded and executed is more important.

I am not sure what to do about the directory visibility. If you think most people would work from reading on the website, then the current way is fine. If they are running the notebook themselves, it may look a bit weird to see the thing repeated with instructions to do something with the directory that has already been done.

(I now understand how the functionality is working: remove-cell metadata from https://myst-nb.readthedocs.io/en/latest/configuration.html.)

felt simpler to have it consistent with files users download.

Yes, consistency is good. It doesn't hurt to leave it as-is.


I should have asked if the pathlib.Path changes were desirable. I saw lots of ...: str | Path when reading the code, which is great; not enough people are using path objects. I figured this would encourage readers to use them.

JaGeo commented 6 months ago

Thank you for your reply, @berquist !

We can surely merge those changes . (@naik-aakash , could you shortly check. I would then mege if you give your okay.)

naik-aakash commented 6 months ago

Hi @berquist , thanks again for these changes! And @JaGeo , I think these can be merged😊

naik-aakash commented 6 months ago

I didn't think of the use case where one downloads only the notebook, which is entirely possible if someone has been following docs and installed LobsterPy from PyPI and not git. I won't touch those instructions then. The commands are advanced but they are short. It is probably not important to explain them since it may be distracting from the tutorial. Mentioning that this documentation page is a Jupyter Notebook that can be downloaded and executed is more important.

I am not sure what to do about the directory visibility. If you think most people would work from reading on the website, then the current way is fine. If they are running the notebook themselves, it may look a bit weird to see the thing repeated with instructions to do something with the directory that has already been done.

(I now understand how the functionality is working: remove-cell metadata from https://myst-nb.readthedocs.io/en/latest/configuration.html.)

felt simpler to have it consistent with files users download.

Yes, consistency is good. It doesn't hurt to leave it as-is.

I should have asked if the pathlib.Path changes were desirable. I saw lots of ...: str | Path when reading the code, which is great; not enough people are using path objects. I figured this would encourage readers to use them.

It's actually good that you spotted this! Somehow it got overlooked :smile:

JaGeo commented 6 months ago

@berquist , I have merged your changes. We will address the other comments as well.

I didn't think of the use case where one downloads only the notebook, which is entirely possible if someone has been following docs and installed LobsterPy from PyPI and not git. I won't touch those instructions then. The commands are advanced but they are short. It is probably not important to explain them since it may be distracting from the tutorial. Mentioning that this documentation page is a Jupyter Notebook that can be downloaded and executed is more important.

I am not sure what to do about the directory visibility. If you think most people would work from reading on the website, then the current way is fine. If they are running the notebook themselves, it may look a bit weird to see the thing repeated with instructions to do something with the directory that has already been done.

(I now understand how the functionality is working: remove-cell metadata from https://myst-nb.readthedocs.io/en/latest/configuration.html.)

felt simpler to have it consistent with files users download.

Yes, consistency is good. It doesn't hurt to leave it as-is.

I should have asked if the pathlib.Path changes were desirable. I saw lots of ...: str | Path when reading the code, which is great; not enough people are using path objects. I figured this would encourage readers to use them.

@naik-aakash , could you follow the suggestions by @berquist and mention that the notebooks can be downloaded in a new PR?