Linopy transition #796
Linopy transition #796
Conversation
|
Currently works locally with
|
|
I can reproduce CI error locally when using |
|
Cool! Just a comment. I would not follow my Linopy PR only but also check if the PyPSA-EUR main changed some parts. |
|
Hello @ekatef, that's great news! :D Regarding solver log, it may make sense to check the latest pypsa-eur implementation as I guess they have accounted for that. As a remark, this morning we talked with Leon and it turns out that to merge this PR requires changes to the -sec version, which may be delicate also for some of their projects. |
|
Hey @davide-f! Thanks a lot for the detailed analysis :) Ah, I see: the load is an input, not a result. So, Regarding a potential merge of this PR, absolutely agree that it's crucial to ensure that the changes introduced don't break anything. Actually, I perceive this work as a kind of experiment to estimate how much effort would be needed for transition to My general impression is that linopy transition is not as challenging as it could be expected from the first sight (thanks to @pz-max PR in PyPSA-Eur and clear instructions) |
|
Some adjustments were done to get closer to PyPSA-Eur implementation. However, According to a warning, there is no |
|
Results of additional testing:
INFO:linopy.solvers:Log file at /tmp/highs.log.
ERROR: getOptionIndex: Option "solver_logfile" is unknown
Presolving model
Problem status detected on presolve: Infeasible
Model status : Infeasible
Objective value : 0.0000000000e+00
HiGHS run time : 0.00
WARNING:linopy.constants:Optimization failed:
Status: warning
Termination condition: infeasible
Solution: 0 primals, 0 duals
Objective: nan
Solver model: available
Solver message: infeasible
|
|
IT may be good to revive this PR and finalize it when possible, to be able to use the latest pypsa version. I am aware that you have a lot on your desk right now, no pressure at all :) we can give priorities and tackle them one by one |
@davide-f agree :) I'd expect that something about a week of focused work should be enough to resolve the remained questions. As discussed, #919 and #903 seem to have higher priorities now. Also, not sure what is the current status of PyPSA-Earth-Sec in terms of Anyway, after #919 and #903 will be finished, happy to get back to this PR. Ping me please in case this would get urgent. |
|
Testing with the updated PyPSA version (0.25.2 is used locally). Clustering is failing currently:
CI is failing due to conda being unhappy: |
|
A couple of notes on
|
Local testing on reproducibility of objective valuemain version
linopy version
That means ~6% difference for the objective function and basically same value for the objective constant (1e-3%, but here is rather a question of numerical precision). So, the PR seems to work properly. As a usability note, that is not obvious at all from the CI logs. The PR changes format of solver traceback due to different conventions of |
|
@finozzifa would be very grateful if you would have time to check this PR. Pre-commit is currently unhappy due to excessively long lines in the configuration file. But that won't be fixed in this PR. Another concern is backward compatibility. I'm currently looking into that, but not sure if it'll be possible to find a code solution for that. |
config.tutorial.yaml
Outdated
| glpk-default: {} # Used in CI | ||
|
|
||
| mem: 30000 #memory in MB; 20 GB enough for 50+B+I+H2; 100 GB for 181+B+I+H2 | ||
| walltime: "12:00:00" |
There was a problem hiding this comment.
In pypsa-eur the option is named "runtime" and has a slightly different naming option, e.g. 12h, though it may be compatible.
Do you think we can adopt the same naming and writing convention?
There was a problem hiding this comment.
Ah, I see! There has been an update in keywords in PyPSA-Eur intended to follow humanfriendly conventions. Ok, let's add this for consistency :)
| @@ -513,6 +513,18 @@ def base_network( | |||
|
|
|||
| if hvdc_as_lines_config: | |||
| lines = pd.concat([lines_ac, lines_dc]) | |||
| lines.drop( | |||
There was a problem hiding this comment.
Generally, I'd recommend to do the converse:
keep the list of columns that we want and implicitly drop the others.
That should be more consistent. However, do you think it should be placed here or maybe in previous processing script?
If you consider that it is not suited for this PR, we can add an issue instead
There was a problem hiding this comment.
Absolutely agree that explicit definition would work better there. Moreover, the updated PyPSA version is not very stable in what relates to the data structures. Custom columns are not forbidden explicitly, but can lead to some weird issues. So, I'd be very careful in dealing with them.
In a dedicated PR, have moved the definition of lines columns in the previous script.
There was a problem hiding this comment.
Great! What would be your idea here, merge this and then 1065, merge 1065 here?
(same text also specified below)
There was a problem hiding this comment.
Have created a PR on top of #1065 which enables linopy in addition to the upgrade of PyPSA version
| @@ -522,6 +534,18 @@ def base_network( | |||
| axis=1, | |||
| result_type="reduce", | |||
| ) | |||
| lines_ac.drop( | |||
There was a problem hiding this comment.
Fixed in #1065: no need for the drop in base_network anymore.
There was a problem hiding this comment.
Great! What would be your idea here, merge this and then 1065, merge 1065 here?
There was a problem hiding this comment.
Great! What would be your idea here, merge this and then 1065, merge 1065 here?
Have created a PR on top of #1065 which enables linopy in addition to the upgrade of PyPSA version
| bus_strategies={ | ||
| "lat": "mean", | ||
| "lon": "mean", | ||
| "tag_substation": "first", | ||
| "tag_area": "first", | ||
| "country": "first", | ||
| }, |
There was a problem hiding this comment.
This is a bit widely duplicated across scripts. v_nom is also not here, may this be somewhat linked to the issue by Emmanuel?
There was a problem hiding this comment.
Agree that the duplication should be fixed. Implemented in #1065 as an update of the aggregation strategies outside the clustering functions.
Yeah, it looks like transparence is much desired in working with v_nom. Have defined "first" aggregation strategy for v_nom to have a better overview on what is put inside the model.
There was a problem hiding this comment.
Good! What would be your idea here, merge this and then 1065, merge 1065 here?
There was a problem hiding this comment.
Good! What would be your idea here, merge this and then 1065, merge 1065 here?
I think it could make sense to have a stacked PR: a PR on top of #1065 which enables linopy in addition to the upgrade of PyPSA version
| # aggregation_strategies = snakemake.params.cluster_options.get( | ||
| # "aggregation_strategies", {} | ||
| # ) | ||
| ## translate str entries of aggregation_strategies to pd.Series functions: | ||
| # aggregation_strategies = { | ||
| # p: {k: getattr(pd.Series, v) for k, v in aggregation_strategies[p].items()} | ||
| # for p in aggregation_strategies.keys() | ||
| # } |
There was a problem hiding this comment.
Can you explain?
No more needed?
There was a problem hiding this comment.
Yes, the format transformation is not needed anymore. The aggregation strategies are read directly from the config via Snakemake parameters.
Have removed the commented-out parts.
| linexpr, | ||
| network_lopf, | ||
| ) | ||
| from pypsa.optimization.abstract import optimize_transmission_expansion_iteratively |
There was a problem hiding this comment.
Have you locally tested the features here changed? I feel like not all of them are parsed into the CI [which is not good too...]
There was a problem hiding this comment.
Yes! I have refactored add_RES_constraints and tested it's behaviour which has (obviously) required to add some fixes to account for linopy grammar.
There may be some advanced stuff which would be nice to investigate in more depth, but on the level of basic functionality it works.
scripts/solve_network.py
Outdated
|
|
||
| logger = create_logger(__name__) | ||
|
|
||
|
|
||
| def prepare_network(n, solve_opts): | ||
| def prepare_network(n, solve_opts, config=None): |
There was a problem hiding this comment.
May you clarify please? Not sure I really get the point 🙂
There was a problem hiding this comment.
I'd propose to have config a mandatory argument, do we need the "=None"?
There was a problem hiding this comment.
Frankly speaking, it has been just a copy-paste from PyPSA-Eur. But agree that it feels more natural to have config as a mandatory argument. Fixed in the stacked PR: this line
@davide-f thanks a lot for the great review! You have made me recognise that PR mixes two different things: PyPSA update and actually linopy transition. Moreover, the updated PyPSA version leads to some odd behaviour in the case when custom columns are present in the components on the network, which should be tracked regardless Have opened #1065 and moved there the changes from this PR which relate to PyPSA update. Would you mind to continue the discussion on the aggregation strategies there? Then, for this PR we can focus on the solving part only. What do you think? |
There was a problem hiding this comment.
I've added few comments; the PR looks in good state; not sure your idea on how to integrate the changes propsoed in 1045.
A weird think I noticed is that the objective function of the problem has significantly changed, from something like 2.6 10^9 to 3.7e+09
Have you noticed it?
scripts/solve_network.py
Outdated
|
|
||
| logger = create_logger(__name__) | ||
|
|
||
|
|
||
| def prepare_network(n, solve_opts): | ||
| def prepare_network(n, solve_opts, config=None): |
There was a problem hiding this comment.
I'd propose to have config a mandatory argument, do we need the "=None"?
| linexpr, | ||
| network_lopf, | ||
| ) | ||
| from pypsa.optimization.abstract import optimize_transmission_expansion_iteratively |
| bus_strategies={ | ||
| "lat": "mean", | ||
| "lon": "mean", | ||
| "tag_substation": "first", | ||
| "tag_area": "first", | ||
| "country": "first", | ||
| }, |
There was a problem hiding this comment.
Good! What would be your idea here, merge this and then 1065, merge 1065 here?
| @@ -522,6 +534,18 @@ def base_network( | |||
| axis=1, | |||
| result_type="reduce", | |||
| ) | |||
| lines_ac.drop( | |||
There was a problem hiding this comment.
Great! What would be your idea here, merge this and then 1065, merge 1065 here?
| @@ -513,6 +513,18 @@ def base_network( | |||
|
|
|||
| if hvdc_as_lines_config: | |||
| lines = pd.concat([lines_ac, lines_dc]) | |||
| lines.drop( | |||
There was a problem hiding this comment.
Great! What would be your idea here, merge this and then 1065, merge 1065 here?
(same text also specified below)
| @@ -12,7 +12,7 @@ dependencies: | |||
| - pip | |||
| - mamba # esp for windows build | |||
|
|
|||
| - pypsa>=0.24, <0.25 | |||
| - pypsa | |||
There was a problem hiding this comment.
Please add >=0.2X in agreement to the expected version; it is not backcompatible unfortunately
| -n.snapshot_weightings.stores * scaling, | ||
| get_var(n, "StorageUnit", "spill").T, | ||
| ) | ||
| # TODO: double check that this is really needed, why do have to subtract the spillage |
There was a problem hiding this comment.
I believe so because in the rhs we have the inflow but not all inflow will be used; spill will be lost.
I don't see charging and discharging efficiencies here, so probably there may be some misalignment because of them.
So this todo should be revised into checking the efficiencies and probably good to raise an issue here.
Colleagues may have experienced minor issues about this maybe
There was a problem hiding this comment.
Agree. I'm also not clear why the inflow and spillage are split in the current formulation which also doesn't make easier implementation of the efficiencies. In any case, we are following here the formulation of PyPSA-Eur, and it could probably make sense to revise both formulations. Happy to open an issue on that
| 0, np.inf, coords=[sns, n.generators.index], name="Generator-r" | ||
| ) | ||
| reserve = n.model["Generator-r"] | ||
| lhs = reserve.sum("Generator") |
There was a problem hiding this comment.
This seems like RES generators can offer reserve too or am I wrong?
There was a problem hiding this comment.
A very interesting point! As far as I understand, yes. We follow PyPSA-Eur in using the approach of GenX when defining the operational reserve constraint. That implies that the reserve margin formulation "optimal dispatch" of RES (whatever it means) and demand response. Have added a short comment on that in the doc string
I fear including wind and solar into the available reserve is against the current regulations in many countries, and it could make sense to have more flexible settings here. Does it sound reasonable for you?
scripts/solve_network.py
Outdated
| "The add_RES_constraints functionality is still work in progress. " | ||
| "Unexpected results might be incurred, particularly if " | ||
| "The add_RES_constraints() is still work in progress. " | ||
| "Unexpected results might be incurre, particularly if " |
There was a problem hiding this comment.
True, have over-done fixing the RES function 😄 Fixed
| .T.groupby(ggrouper, axis=1) | ||
| .apply(join_exprs) | ||
| (n.model["Generator-p"].loc[:, gens_i] * n.snapshot_weightings.generators) | ||
| # .groupby(ggrouper.to_xarray()) |
There was a problem hiding this comment.
I had to stop here as groupby() has a bit specific dynamics in linopy and feels like a feature under development. In the current version I have not been to make it work, but the behaviour of groupby() has just changed recently, and more changes can be expected, as far as I can understand.
Moreover, I'm not quite sure this grouping is really needed. Currently, a share of RES is set for each country in a dedicated constraint, but this share is same for all the countries. I believe there may be some room for improvement here which we can track as a dedicated issue, as that is quite a frequent request to improve representation of RES development plans
|
Thanks a lot for the in-depth review @davide-f! Along the way, we have identified a number of the additional issues, both related to this PR and completely independent ones. The list looks like follows
Regarding the overall implementation strategy, my feeling is that it makes sense to disentangle The PyPSA update leads to ~1% change of the objective function, while enabling linopy leaves the objective function pretty much the same (~1e-5 is the relative difference). So, I think this work is close to be finished, keeping in mind the points above 🙂 |
|
Closing in favour #1169 |
Closes #494
Integrate
linopyfollowing PyPSA/pypsa-eur#625Checklist
envs/environment.yamlanddoc/requirements.txt.config.default.yamlandconfig.tutorial.yaml.test/(note tests are changing the config.tutorial.yaml)doc/configtables/*.csvand line references are adjusted indoc/configuration.rstanddoc/tutorial.rst.doc/release_notes.rstis amended in the format of previous release notes, including reference to the requested PR.