IPESE / REHO

Master version of the model
Apache License 2.0
6 stars 10 forks source link

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

Closed nmstreethran closed 1 month ago

nmstreethran commented 5 months ago

https://github.com/openjournals/joss-reviews/issues/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 https://github.com/IPESE/REHO/issues/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 commented 5 months ago

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 commented 4 months ago

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 commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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 (https://github.com/IPESE/REHO/commit/b7d869afd4eb847f8a04184b8e602d6133694886), 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 (https://github.com/IPESE/REHO/commit/fae50970a0484d7782b4d71e0e9852b23143fed0)