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

Restructure CHILLED code to be more trackable and customizable #18

Merged
merged 114 commits into from
Aug 21, 2024

Conversation

measrainsey
Copy link
Contributor

@measrainsey measrainsey commented Jul 10, 2024

Reorganize CHILLED code to be more package-friendly

Main changes are:

  • Function (input/output) driven code for more package-friendly development and not just scripts
  • Usage of Config object for more consistent default inputs
  • Inputs housed in Github for traceability and flexibility
  • Can be run from command line (takes arguments as inputs)
  • Runnable on UniCC cluster

Changes to inputs and parameters

  • There now exists a Config object (in utils.config) that has the default parameters that should be consistently used in the model, but now we can easily change each parameter via the Config object instead of looking for it in places across different scripts.
  • For each "version" of the model (such as "ALPS2023"), make sure a folder with the version name exists in data/chilled/version with the following files

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:

  • Version name (default: "ALPS2023")
  • GCM scenario (default: "GFDL-ESM4")
  • RCP (default: "baseline")

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
Copy link
Member

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

  • There were some dependencies missing in pyproject.toml (which I noticed since I started in a new venv)
  • 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.

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 :)

Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now looks good from my side.

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.

Now for @amastrucci, @byersiiasa, or probably @Zh-xy: please review and approve the content-side of the changes :)

@measrainsey
Copy link
Contributor Author

measrainsey commented Jul 30, 2024

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
Copy link
Member

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
Copy link
Member

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

measrainsey commented Aug 1, 2024

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

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
Copy link
Member

Its only for Fridolin to approve ;)

@measrainsey
Copy link
Contributor Author

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

Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost, yeah, that's why I approved this PR before :)

Since you asked for a new review, I've looked again at the files your recent commits changed and found some hints and questions that I hope are useful. I trust you will handle them well, so I will approve this PR again and once you're satisfied with your actions responding to the new comments, please feel free to merge :)

message_ix_buildings/chilled/preprocess/message_raster.py Outdated Show resolved Hide resolved
message_ix_buildings/chilled/run_agg.py Outdated Show resolved Hide resolved
message_ix_buildings/chilled/run_main.py Outdated Show resolved Hide resolved
message_ix_buildings/chilled/run_preprocess.py Outdated Show resolved Hide resolved
message_ix_buildings/chilled/util/config.py Outdated Show resolved Hide resolved
@measrainsey measrainsey merged commit 6cde97d into main Aug 21, 2024
@measrainsey measrainsey deleted the chilled/restruct branch August 21, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chilled PRs and issues related to the CHILLED model documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants