-
Notifications
You must be signed in to change notification settings - Fork 3
Mkdocs setup & CLI implementation (etc etc) #51
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
base: main
Are you sure you want to change the base?
Conversation
EllieKallmier
left a comment
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.
Sweet looks so solid! Few qs as usual for my benefit :)
docs/config.md
Outdated
|
|
||
| The order in which different weather reference years are used in the model. If the | ||
| number of reference years is less than the number of model years the reference years | ||
| will be reused in the order provided. |
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.
Maybe helpful to give explicity example of this, e.g. if there are 5 model years and 2 ref years given (A, B) the reference year cycle becomes [A, B, A, B, A] etc.
src/ispypsa/cli/dodo.py
Outdated
| ) | ||
|
|
||
| # Get dont_run flag from doit variables | ||
| dont_run = get_var("dont_run_capacity_expansion", "False") == "True" |
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.
this is a little thing and more on the semantic side (+is just opinion) - but I think to me it would make more sense to phrase and implement this flag more as an opt-out than and opt-in?
e.g., instead of a "dont_run" flag with var name "dont_run_capacity_expansion", phrasing as a "run_optimisation" flag with var name "run_optimisation_for_capacity_expansion" (or something) feels a little clearer to me. And obv default is true and manually set to false means the opt doesn't run.
I think I'm just having an aversion to something that feels close to a double negative here so am very happy to be overruled - plus this already works so :)
| ValueError: If both or neither of nem_regions and isp_sub_regions are provided | ||
| """ | ||
| # Validate inputs | ||
| if (nem_regions is None) == (isp_sub_regions is None): |
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.
For my understanding broader validation practices: the 'both true' version of this condition is also validated before this function gets called in the create_ispypsa_inputs_template function. Is there a schema/reason for kind of splitting up validation in this way? Thinking about how I'm sometimes indecisive about where's the best/right/good place to validate this kind of thing and whether there's a handy rule or thought process I can adopt :)
|
|
||
| if not selected_sub_regions: | ||
| logging.warning("No valid regions found for filtering") | ||
| return [], [] |
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.
maybe similar question to above (just a q for me) - is it generally preferred/better practice to e.g. return values here that later (after this function has been called and run) trigger an error rather than raising the error here?
I'm imagining it's probably in conjunction with logging helpful for tracing the error back if it's getting raised in the higher-level function?
| @@ -1,6 +1,2 @@ | |||
| Region,CCGT,CCGT with CCS,Small OCGT,Large OCGT,Reciprocating engines,Hydrogen Reciprocating engines,Biomass,1 hr Battery Storage,2 hr Battery Storage,4 hr Battery Storage,8 hr Battery Storage,Pumped Hydro3 (8 hrs storage),Pumped Hydro (24 hrs storage),Pumped Hydro (48 hrs storage),BOTN- Cethana | |||
| QLD,110.49433333333297,110.49433333333297,110.49433333333332,110.49433333333332,110.49433333333332,110.49433333333332,110.49433333333332,103.36566666666667,103.36566666666667,103.36566666666667,103.36566666666667,0,0,0,0 | |||
| Region,CCGT,CCGT with CCS,Small OCGT2,Large OCGT,Reciprocating engines,Hydrogen Reciprocating engines,Biomass,1 hr Battery Storage,2 hr Battery Storage,4 hr Battery Storage,8 hr Battery Storage,Pumped Hydro3 (8 hrs storage),Pumped Hydro (24 hrs storage),Pumped Hydro (48 hrs storage),BOTN- Cethana | |||
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.
looks like footnote back here too? (Matching?)
for more information, see https://pre-commit.ci
Co-authored-by: EllieKallmier <[email protected]>
f49b893 to
bad0ece
Compare
Summary
This PR adds:
Command-Line Interface (CLI)
New ispypsa command-line interface built on doit:
create_pypsa_friendly_inputs, create_and_run_capacity_expansion_model, create_operational_timeseries,
create_and_run_operational_model
Regional Filtering Feature
Added capability to filter ISPyPSA templates by region or sub-region:
Translator & Snapshots Changes
Updates to PyPSA translation and snapshot handling:
Testing Data Updates
Updated test IASR data and trace files: