-
Notifications
You must be signed in to change notification settings - Fork 273
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
Tighten constraint on usage of global vars #1549
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR aims to tighten constraints on the usage of global variables and to improve function interfaces for better modularity. Key changes include updating function signatures (e.g. adding a new social discount parameter and passing through the current investment years), and consistently adding pylint disable comments (E0606) for calls to configure_logging.
Reviewed Changes
File | Description |
---|---|
scripts/solve_operations_network.py | Added pylint disable comment when calling configure_logging. |
scripts/prepare_perfect_foresight.py | Updated concat_networks and set_all_phase_outs signatures and their calls. |
scripts/add_brownfield.py | Added pylint disable comment when calling configure_logging. |
scripts/prepare_osm_network_release.py | Updated create_geometries signature to include is_converter and logging fix. |
scripts/add_existing_baseyear.py | Replaced a log warning with a raise ValueError for undefined heat systems. |
scripts/add_transmission_projects_and_dlr.py | Added pylint disable comment when calling configure_logging. |
scripts/add_electricity.py | Added pylint disable comment and introduced an explicit error branch. |
.github/workflows/test.yaml | Updated pylint command to enable E0606 along with E0601. |
scripts/prepare_network.py | Added pylint disable comment when calling configure_logging. |
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
scripts/prepare_perfect_foresight.py:626
- [nitpick] Consider renaming the parameter 'social_discountrate' to 'social_discount_rate' for improved readability and clarity.
n = concat_networks(years, network_paths, social_discountrate)
scripts/add_existing_baseyear.py:460
- Replacing a log warning with an exception can affect control flow; please ensure this exception is properly handled in callers so that unexpected termination is avoided.
raise ValueError(f"Heat system {heat_system} not defined.")
scripts/add_electricity.py:775
- [nitpick] Ensure that all valid cases for 'hydro_max_hours' are covered upstream; this exception should only be raised when an unexpected method is encountered to prevent unintended termination.
raise ValueError(f"Unknown hydro_max_hours method: {hydro_max_hours}")
@fneum fine with merging? (copilot at least seems to be) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The docstrings are automated, right? Did you double-check that they are correct? Sometimes I felt they were a bit too wordy, hard to maintain.
I'll merge now. However, we should generally aim to edit AI-suggested docstrings for conciseness. Passing of some arguments could be more specific (e.g. when only one particular element of |
Thanks @fneum. Totally agree. It was on my list to review it, but I think it's fine for now. |
Follow up on #1537
Changes proposed in this Pull Request
Some cases were not checked by the CI. With this we should be save to easily distribute functions across modules
Checklist
envs/environment.yaml
.config/config.default.yaml
.doc/configtables/*.csv
.doc/data_sources.rst
.doc/release_notes.rst
is added.