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

[JOSS review] Improvements to "Getting started" in docs #21

Closed
nmstreethran opened this issue May 16, 2024 · 5 comments
Closed

[JOSS review] Improvements to "Getting started" in docs #21

nmstreethran opened this issue May 16, 2024 · 5 comments

Comments

@nmstreethran
Copy link

nmstreethran commented May 16, 2024

openjournals/joss-reviews#6734

In your documentation, you start off with two alternatives for installing REHO: PyPI and from source. However, the 'Requirements' section only talks about installing REHO from source. I suggest slightly reorganising your steps as follows:

  1. Prerequisites - Python 3 and AMPL installations
  2. Clone the repository and set up the AMPL licence path and key
  3. Installation:
    • from PyPI
    • from source (and checking if it's installed properly)
  4. Running REHO

Furthermore, you should recommend users to install the package in a venv (you have stated this in the README but not in the docs). Also, the example.env file in the docs has only one variable (AMPL_PATH), but the one included in the REHO repo has two (AMPL_PATH and API_VESE_KEY).

Regarding the relative path issue which you have documented in #13, I think this is not limited to VS Code. If I run your scripts in a standalone terminal, I still encounter the issue. Also, setting "python.terminal.executeInFileDir": true didn't work for me as it produced FileNotFoundError: The relative path that was given is not a valid file.. Do you have a solution for this? I think terminals usually expect the paths to be defined relative to the working directory. I was able to run your scripts by first doing cd scripts/template/ and then python run.py and python plot.py.

I was able to run run.py and plot.py when REHO is installed from source, but not when it's installed from PyPI. It looks like the PyPI version is out of date and is missing dependencies, i.e. coloredlogs and pvlib.

Additionally, when I run run.py, the following warning occurs:

/mnt/Backup/Downloads/REHO/reho/model/preprocessing/QBuildings.py:436: UserWarning: Geometry column does not contain geometry.
  new_buildings_data[REHO_index] = df_buildings[key]
@DorsanL
Copy link
Collaborator

DorsanL commented May 17, 2024

Thank you, your recommendations have been carefully considered!

I suggest slightly reorganising your steps as follows

A new structure has been set for Getting started, based on your suggestions.

you should recommend users to install the package in a venv

Yes, this has been precised.

API_VESE_KEY in .env

This API_VESE_KEY relates to a specific example (scripts/examples/3i_Electricity_prices.py) where we connect to an API to extract electricity prices from a public database.
This environment variable is not crucial. If it's not defined, the function get_vese_key() from reho/model/preprocessing/electricity_prices.py will retrieve the key from a URL (I just need to conduct some tests to verify it is accessible outside EPFL intranet).

I think terminals usually expect the paths to be defined relative to the working directory.

I will discuss this with a colleague and try to enhance the documentation regarding such issue. Nice that you still managed to run the scripts!

I was able to run run.py and plot.py when REHO is installed from source, but not when it's installed from PyPI. It looks like the PyPI version is out of date and is missing dependencies, i.e. coloredlogs and pvlib.

This is correct, the package had not been updated for quite some time. A new REHO 1.1.0 version is now available, I hope everything will work fine.

@nmstreethran
Copy link
Author

I have the following additional suggestions for your documentation.

  1. Consider adding a Jupyter notebook for the scripts in the template directory and including that in your Sphinx documentation. I managed to run those scripts in a notebook which you can view here: https://nbviewer.org/gist/nmstreethran/2475549ab51b02c70eb48ad438781ea1/template.ipynb. I think this could be a nice way to display all the expected results and the interactive plots within the documentation.

  2. Consider including titles and additional context (e.g. explanation of acronyms) in the interactive plots. Currently, only the time series plot in the template scripts has a title.

  3. There are some boxes / annotations (for example, here and here ("TO DO")) which should probably be commented out.

  4. Consider adding a CONTRIBUTING.md file in your GitHub repository and copying the contents of contributing guidelines.

I have created a separate issue for the warnings that appeared when running the scripts at #24.

@DorsanL
Copy link
Collaborator

DorsanL commented Jul 23, 2024

Thank you for these suggestions!

  1. Right, that's an interesting idea. Just to make sure I understand: are you suggesting integrating the notebook into the repository (and not into the package directly), and as well displaying its contents directly in the documentation? (I'll have to see if that's possible for a "for a direct integration", or if it has to go through an external link like yours)
  2. I understand. My choice to work with html files allows me to generally adjust all this in post-processing, but I can add optional title and context.
  3. This has been addressed.
  4. I added a "Contribute" paragraph in the README.md, which redirects to the [Contribute](https://reho.readthedocs.io/en/main/sections/7_Contribute.html section) section of the documentation. (I thought it would be redundant and less efficient for maintenance to copy the content? but if you would prefer to have a separate CONTRIBUTING.md file, I can add it)

@nmstreethran
Copy link
Author

Right, that's an interesting idea. Just to make sure I understand: are you suggesting integrating the notebook into the repository (and not into the package directly), and as well displaying its contents directly in the documentation? (I'll have to see if that's possible for a "for a direct integration", or if it has to go through an external link like yours)

There are ways to integrate Jupyter notebooks with Sphinx. See https://docs.readthedocs.io/en/stable/guides/jupyter.html. You can consider these in the future to enhance the documentation. This is not a requirement for the review.

I understand. My choice to work with html files allows me to generally adjust all this in post-processing, but I can add optional title and context.

There are a number of plots generated when running the examples and a title (at least) would provide some context to users to better interpret the results.

I added a "Contribute" paragraph in the README.md, which redirects to the [Contribute](https://reho.readthedocs.io/en/main/sections/7_Contribute.html section) section of the documentation. (I thought it would be redundant and less efficient for maintenance to copy the content? but if you would prefer to have a separate CONTRIBUTING.md file, I can add it)

This is good enough.

@DorsanL
Copy link
Collaborator

DorsanL commented Aug 30, 2024

There are ways to integrate Jupyter notebooks with Sphinx. See https://docs.readthedocs.io/en/stable/guides/jupyter.html. You can consider these in the future to enhance the documentation. This is not a requirement for the review.

A Jupyter Notebook has now been included in the documentation (b7d869a), and is visible here: https://reho.readthedocs.io/en/main/sections/Notebook.html

There are a number of plots generated when running the examples and a title (at least) would provide some context to users to better interpret the results.

All the functions in plotting.py now have a title argument, and examples have been enhanced accordingly (fae5097)

@DorsanL DorsanL closed this as completed Aug 30, 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

No branches or pull requests

2 participants