iiasa / message-ix-buildings

Apache License 2.0
2 stars 3 forks source link

Restructure CHILLED code to be more trackable and customizable #18

Closed measrainsey closed 3 months ago

measrainsey commented 4 months ago

Reorganize CHILLED code to be more package-friendly

Main changes are:

Changes to inputs and parameters

Changes to usage

Instead of modifying scripts that has parameters set in the script, now the parameters can be set via the Config object and modified via arguments. Currently, using the run scripts, these are the arguments that be given from the command line:

Preprocessing of rasters can be done first without considering GCM and RCP. This can be accomplished by running the script (in /chilled/):

python -m run_preprocess.py -version "version_name"

The part of the model that collects and processes the climate data to derive energy intensities can be run using:

python -m run_main.py -version "version_name" -gcm "gcm_name" -rcp "rcp_name"

Note that the code is now also easily runnable on the UniCC server, which is nice for setting off multiple GCM-RCP runs at once.

How to review

For @amastrucci or @byersiiasa: please let me know if the code does not make sense now or if the usage is not intuitive/efficient.

Perhaps @glatterf42 can also review if the code structure and docs are okay.

Thank you!

glatterf42 commented 4 months ago

Quickly jumped in here to understand what's going on with the docs: overall, you were moving in the right direction.

With just these two changes, the docs now built successfully for me, including the code reference part. To replicate (in your venv on a command line of your choice):

  1. Go to doc/
  2. Run a command like sphinx-build -T -E -b html -d _build/doctrees-readthedocs -D language=en . _build/html
  3. Host your own server via python -m http.server (and stop with ctrl+c once you're done)
  4. Access the link it shows with your favourite browser (something like http://0.0.0.0:8000/)
  5. Navigate to _build/html/ and have a look around :)
measrainsey commented 4 months ago

Thanks @glatterf42 for your review and edits! A follow-up:

  • Sphinx could not understand some of the imports. E.g. importing from util.config in message_ix_buildings/chilled/preprocess/message_raster.py makes sphinx look at all top-level modules (like pandas, numpy, or message_ix_buildings) and it doesn't find util there. So we can either use absolute paths (like I did now) or relative paths. This would then be from .util.config import Config in this case. Not entirely sure which one is preferable.

Using the absolute paths is giving me an issue now when trying to run the python scripts from the command line/terminal. For example, with using absolute paths, I am now receiving the error:

  File "/home/mengm/repo/message-ix-buildings/message_ix_buildings/chilled/preprocess/message_raster.py", line 12, in <module>
    from message_ix_buildings.chilled.util.config import Config
ModuleNotFoundError: No module named 'message_ix_buildings'

Within message_raster.py, if I change the import line to use the relative path from .util.config import Config, I receive the following error when running the script in command line:

  File "/Users/meas/iiasagit/message-ix-buildings/message_ix_buildings/chilled/preprocess/message_raster.py", line 12, in <module>
    from .util.config import Config
ModuleNotFoundError: No module named 'preprocess.util'

I figure I needed to move the relative path to the directory before the preprocess submodule, but if I tried to change the import line to from ..util.config import Config instead, I receive the following error:

  File "/Users/meas/iiasagit/message-ix-buildings/message_ix_buildings/chilled/preprocess/message_raster.py", line 12, in <module>
    from ..util.config import Config
ImportError: attempted relative import beyond top-level package

Do you have any suggestions on how to proceed? The only import line that seems to work in order to run the script in command line is from util.config import Config, but that's causing issues for the documentation build.

Thanks!

glatterf42 commented 3 months ago

Huh, that sounds odd. My first thought was that this might be dependent on where you are when you execute the scripts, but this would rather be an issue with relative imports than with the absolute paths we have now. My next guess would be that you might not have an editable install of message-ix-buildings, but if you produce various error messages after only changing one line, you likely do have an editable install. The (maybe unfortunate) thing is: I can't reproduce your error messages. For me, this is what I see:

fridolin@fridolin-Latitude-5520 ~> (buildings) cd message-ix-buildings/
fridolin@fridolin-Latitude-5520 ~/message-ix-buildings (chilled/restruct)> (buildings) python
Python 3.12.4 (main, Jun  8 2024, 18:29:57) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from message_ix_buildings.chilled.util.config import Config
>>> 
fridolin@fridolin-Latitude-5520 ~/message-ix-buildings (chilled/restruct)> (buildings) cd message_ix_buildings
fridolin@fridolin-Latitude-5520 ~/m/message_ix_buildings (chilled/restruct)> (buildings) python
Python 3.12.4 (main, Jun  8 2024, 18:29:57) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from message_ix_buildings.chilled.util.config import Config
>>> 
fridolin@fridolin-Latitude-5520 ~/m/message_ix_buildings (chilled/restruct)> (buildings) cd chilled
fridolin@fridolin-Latitude-5520 ~/m/m/chilled (chilled/restruct)> (buildings) python
Python 3.12.4 (main, Jun  8 2024, 18:29:57) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from message_ix_buildings.chilled.util.config import Config
>>> 
fridolin@fridolin-Latitude-5520 ~/m/m/chilled (chilled/restruct)> (buildings) python preprocess/message_raster.py 
fridolin@fridolin-Latitude-5520 ~/m/m/chilled (chilled/restruct)> (buildings) git status
On branch chilled/restruct
Your branch is up to date with 'origin/chilled/restruct'.

nothing to commit, working tree clean
fridolin@fridolin-Latitude-5520 ~/m/m/chilled (chilled/restruct)> (buildings) cd ../..
fridolin@fridolin-Latitude-5520 ~/message-ix-buildings (chilled/restruct)> (buildings) python message_ix_buildings/chilled/preprocess/message_raster.py
fridolin@fridolin-Latitude-5520 ~/message-ix-buildings (chilled/restruct)> (buildings)

In other words: wherever I am on my system, I can import from message_ix_buildings as from any other package and I can execute message_raster.py without error.

So just to clarify: you pulled the latest changes from here (especially including new __init__ files) and this directly adapted your install of message-ix-buildings because you installed it via pip install -e ...? What are you using to manage your venv? Is the correct venv active? If you run pip list, do you see message-ix-buildings in that list? With a version like 0.0.2.dev7+gd68f29b.d20240725? And finally, which OS are you using?

byersiiasa commented 3 months ago

Hi all, thanks for this, looks very good. Main suggestion would be to add a bit more intro to the README to explain more about CHILLED, cite relevant papers, and explain a bit more about the input data types and outputs that are generated.

measrainsey commented 3 months ago

Hi all, thanks for this, looks very good. Main suggestion would be to add a bit more intro to the README to explain more about CHILLED, cite relevant papers, and explain a bit more about the input data types and outputs that are generated.

Thanks Ed! If it's alright, my preference at the moment would be to get this PR merged and then add the documentation in another PR (so that this one does not get too large). 🙂 I have created an issue for the documentation: #21

If all else looks good, could you submit an approval? Otherwise, feel free to note other changes you would like made!

measrainsey commented 3 months ago

Code could still be cleaned up (e.g. by adding mypy and ruff like we have them in message_ix and addressing linting/formatting errors or by cleaning up comments and type: ignore statements), but this could also happen in a later PR. If saved for later, please open an issue to not forget about this.

As a note, I just created an issue for this: #22

byersiiasa commented 3 months ago

Its only for Fridolin to approve ;)

measrainsey commented 3 months ago

@glatterf42 If it's okay could I go ahead and merge? :)