Linopy Integration #351

GbotemiB · 2024-06-24T21:47:53Z

Closes # (if applicable).

Changes proposed in this Pull Request

Integrate linopy following PyPSA/pypsa-eur#625 and pypsa-meets-earth/pypsa-earth#796

Checklist

I tested my contribution locally and it seems to work fine.
Code and workflow changes are sufficiently documented.
Newly introduced dependencies are added to envs/environment.yaml and envs/environment.docs.yaml.
Changes in configuration options are added in all of config.default.yaml, config.tutorial.yaml, and test/config.test1.yaml.
Changes in configuration options are also documented in doc/configtables/*.csv and line references are adjusted in doc/configuration.rst and doc/tutorial.rst.
A note for the release notes doc/release_notes.rst is amended in the format of previous release notes, including reference to the requested PR.

ekatef · 2024-06-27T10:24:29Z

Thanks a lot for opening the PR! Currently, CI is failing, and it looks like the reason is the recent environment troubles we experienced in PyPSA-Earth. Those troubles have been fixed recently with this PR and merged into Linopy PR in PyPSA-Earth. Could you please change the update the commit on which your PR relays? I think, this one should work fine.

GbotemiB · 2024-06-27T12:21:36Z

Hi @ekatef,

Thanks for looking into this. I will review the changes and revert.

ekatef · 2024-06-27T12:30:05Z

Hi @ekatef,

Thanks for looking into this. I will review the changes and revert.

Hey @GbotemiB, no worries and no need to revert! 🙂 All good, and we just need to include the recent changes form PyPSA-Earth upstream.

ekatef · 2024-06-27T16:30:39Z

scripts/solve_network.py

-    if co2_stores.empty or ("Store", "e") not in n.variables.index:
+    if co2_stores.empty:  # or ("Store", "e") not in n.variables.index:


Just for better understanding: what has been the reason of commenting-out the second part of the conditon?

The variables method no longer exists in the network, which is why I commented it out. I also noticed something in pypsa-eur implementation.

You mean n.variables component, right?

Agree that some adjustment may be needed, but we have to avoid losses in functionality. It would be great to understand the purpose of having ("Store", "e") not in n.variables.index condition here, check if it's still needed and find a way to re-implement it. May be a good point to discuss with the cross-sectoral team.

I don't know the reason or the functionality. But, I checked similar implementations in pypsa-eur, before the linopy integration, and after the integration, and it seems to have been done in the same way.

scripts/solve_network.py

ekatef · 2024-06-27T16:38:08Z

Thanks a lot @GbotemiB! The update has worked 🎉
CI is failing currently but the reason is that some remote source is unavailable (urllib.error.HTTPError: HTTP Error 503: Service Unavailable).

The PR itself looks great! Is it ready for review?

GbotemiB · 2024-06-27T17:07:28Z

@ekatef, the PR is ready for review. I didn't mark it for review because the Linopy PR in pypsa-earth is still in draft.

I have investigated the error in the CI. It relates to prepare_transport_data_input:download_number_of_vehicles.py which is supposed to access this link here, but at the moment the site is currently not reachable. This should be the reason for the error.

davide-f

Hello :D
Just adding few cents :)
Great @GbotemiB :D

davide-f · 2024-07-01T10:09:39Z

config.default.yaml


  mem: 30000 #memory in MB; 20 GB enough for 50+B+I+H2; 100 GB for 181+B+I+H2
+  walltime: "12:00:00"


In pypsa-eur, the standard parameter is "runtime", may be good to revise?

Thanks @davide-f, I will check it out.

davide-f · 2024-07-02T07:27:03Z

scripts/solve_network.py

+        lhs = (
+            (1 * link_p.loc[heat_ext]),
+            (1 * link_p.loc[electric_ext]),
+            (1 * link_p_nom.loc[electric_ext]),
        )


This one cannot work.
It feels to me that probably we need to slightly change the CI options to enable more of these functions to actually be tested.
Moreover, pay attention on signs, it seems something has went wrong here

Thanks @davide-f for catching this. I will revise the implementation.

for more information, see https://pre-commit.ci

ekatef · 2024-07-03T09:16:46Z

scripts/solve_network.py

-    vars_final_co2_stored = get_var(n, "Store", "e").loc[sns[-1], co2_stores]
+    vars_final_co2_stored = n.model["Store-e"].loc[sns[-1], co2_stores]


As a general comment, indexing in linopy may be a bit delicate. My observation is that there may be no one-to-one correspondence with old functions. Some testing may be needed, especially there is no exact match with PyPSA-Eur.

Do you have any ideas on how we can test the functionalities?

from my end, I think we can test the original functionalities alongside the new ones, and compare the results.

ekatef · 2024-07-03T09:21:06Z

scripts/solve_network.py

-    lhs = linexpr((1, vars_final_co2_stored)).sum()
+    lhs = (1 * vars_final_co2_stored).sum()


Not sure we need to multiply by 1 here and in all similar fragments.

My understanding is that a linopy equivalent of linexpr((1, vars_final_co2_stored)).sum() is vars_final_co2_stored.sum(). But that is not necessarily 100% correct understanding, and happy to discuss. Do you have any ideas on that? Have you tested these parts?

You are right. The 1 multiple is redundant. I did a bit of debugging to verify the expression already. However, I am open to more ways to test these implementations.

ekatef · 2024-07-03T09:39:15Z

Hello @GbotemiB!

Very nice and quite comprehensive work! Thank you for addressing the review comments. I think, now we have a good basic linopy-based implementation both for pypsa-earth and pypsa-earth-sec. The next step must be a good testing, as linopy is quite a powerful tool which should be used in a mindful way. Along with PyPSA-Eur implementation, there are great examples on linopy usage which can be very helpful to get the main principles. I have also collected some observations on linopy syntax which may be relevant for our PRs: see this comment.

Absolutely agree with @davide-f that CI doesn't catch the points which are crucial for functionality of both linopy PRs. Which mean that we can't fully relate on CI in this case, and some a manual testing is necessary. My feeling is that the following tests are needed:

Execute some extra-functionality functions. In particular, add_chp_constraints(n) looks quite complicated, and it's local testing is highly desirable.
Check that the objective didn't change due to this PR. Or didn't change significantly, at the very least :)
Backward compatibility is respected: PR runs also with the old environment.

@GbotemiB does the plan sound reasonable for you?

GbotemiB · 2024-07-03T11:15:40Z

Execute some extra-functionality functions. In particular, add_chp_constraints(n) looks quite complicated, and it's local testing is highly desirable.

Check that the objective didn't change due to this PR. Or didn't change significantly, at the very least :)

Backward compatibility is respected: PR runs also with the old environment.
@GbotemiB does the plan sound reasonable for you?

@ekatef Sounds perfect. I think in terms of priority, 1 should come ontop.

GbotemiB · 2024-07-03T13:05:01Z

Check that the objective didn't change due to this PR. Or didn't change significantly, at the very least :)

Execute some extra-functionality functions. In particular, add_chp_constraints(n) looks quite complicated, and it's local testing is highly desirable.

Check that the objective didn't change due to this PR. Or didn't change significantly, at the very least :)

Backward compatibility is respected: PR runs also with the old environment.
@GbotemiB does the plan sound reasonable for you?

@ekatef Sounds perfect. I think in terms of priority, 1 should come on top.

I should also mention here that in the original pypsa-earth-sec, the test config results in nan as objective value. I think this is one of the things that CI doesn't catch.

GbotemiB · 2024-07-12T12:25:10Z

Hello @GbotemiB!

Very nice and quite comprehensive work! Thank you for addressing the review comments. I think, now we have a good basic linopy-based implementation both for pypsa-earth and pypsa-earth-sec. The next step must be a good testing, as linopy is quite a powerful tool which should be used in a mindful way. Along with PyPSA-Eur implementation, there are great examples on linopy usage which can be very helpful to get the main principles. I have also collected some observations on linopy syntax which may be relevant for our PRs: see this comment.

Absolutely agree with @davide-f that CI doesn't catch the points which are crucial for functionality of both linopy PRs. Which mean that we can't fully relate on CI in this case, and some a manual testing is necessary. My feeling is that the following tests are needed:

Execute some extra-functionality functions. In particular, add_chp_constraints(n) looks quite complicated, and it's local testing is highly desirable.

Check that the objective didn't change due to this PR. Or didn't change significantly, at the very least :)

Backward compatibility is respected: PR runs also with the old environment.

@GbotemiB does the plan sound reasonable for you?

About testing the implementation of add_chp_constraints(n), I confirmed that the implementation Is now just as it is in pypsa-eur. I havent found a way to test it, I have a plan to run the script in debug mode and test it that way.
I have confirmed that the objective didn't change due to this PR. I ran the CI locally which returns a nan for the objective value. I am already in the process of investigating why this happened with @finozzifa.
The current PR can't run with the old environment because of the latest version of pypsa.

davide-f

Few comments; great @GbotemiB! :D
As a comment, do you feel like by simply changing few parameters in the config test yaml, the functions you modified could be included in the CI?
That may ease the process (if we also fix the CI, but that may not be done here)

davide-f · 2024-07-16T15:51:31Z

Snakefile

-        clustered_pop_layout="resources/population_shares/pop_layout_elec_s{simpl}_{clusters}_{planning_horizons}.csv",
+        clustered_pop_layout="resources/population_shares/pop_layout_elec_s{simpl}_{clusters}.csv",


Please, check the main, probaly one merge conflict didn't go well

this applies for changes below too

Thanks for catching this. I have resolved the merge conflict.

davide-f · 2024-07-16T15:53:57Z

scripts/solve_network.py

+    h2_network_cap = n.model["Link-p_nom"]
+    lhs = (h2_network_cap.loc[h2_network.index] * h2_network.length).sum()


Please, check the merge conflict; an if-case has been dropped unfortunately

GbotemiB · 2024-07-18T02:34:45Z

Few comments; great @GbotemiB! :D As a comment, do you feel like by simply changing few parameters in the config test yaml, the functions you modified could be included in the CI? That may ease the process (if we also fix the CI, but that may not be done here)

Hi @davide-f, could you please clarify what you mean here?
Do you mean making changes to the config test file?

davide-f · 2024-07-18T08:37:03Z

Hi @davide-f, could you please clarify what you mean here? Do you mean making changes to the config test file?

Some functions in the solve_network are triggered by options in the config file, some of them may be off by default which means that they are not tested.
Not a priority, but worth mentioning if it may help the revision

GbotemiB · 2024-07-18T14:56:22Z

Hi @davide-f, could you please clarify what you mean here? Do you mean making changes to the config test file?

Some functions in the solve_network are triggered by options in the config file, some of them may be off by default which means that they are not tested. Not a priority, but worth mentioning if it may help the revision

The option to test the add_chp_contraints in the test config is already set to true. The issue is that the solve_network script doesn't call the function. I have included it in my PR.

GbotemiB added 2 commits June 24, 2024 22:27

modified files to adjust to linopy

40f0739

removed unused comments

d6937f9

GbotemiB marked this pull request as draft June 24, 2024 21:48

added config to include new parameters

c8657fb

GbotemiB added 2 commits June 27, 2024 16:09

config tailored to accomodate new functionalities

39cd0c4

merge upstream for pypsa-earth

797f90c

ekatef reviewed Jun 27, 2024

View reviewed changes

scripts/solve_network.py Outdated Show resolved Hide resolved

GbotemiB marked this pull request as ready for review June 27, 2024 17:23

removed rolling horizon option

fabc5f4

davide-f reviewed Jul 2, 2024

View reviewed changes

pre-commit-ci bot and others added 4 commits July 2, 2024 17:15

[pre-commit.ci] auto fixes from pre-commit.com hooks

796890c

for more information, see https://pre-commit.ci

merge from ekatef dev-linopy branch

244bf36

update config variable

130179d

updates signs

ebbe056

ekatef reviewed Jul 3, 2024

View reviewed changes

davide-f mentioned this pull request Jul 4, 2024

Prepare the MERGE #312

Open

18 tasks

modified chp constraints

80be38b

GbotemiB added 2 commits July 15, 2024 14:45

precommit fixes

415239f

add_chp_constraints fixed and tested

ab7188a

added chp to extra_functionalities

f7b8e38

GbotemiB force-pushed the linopy-dev branch 2 times, most recently from dd3102c to b4c4b74 Compare July 15, 2024 14:36

davide-f reviewed Jul 16, 2024

View reviewed changes

GbotemiB added 3 commits July 17, 2024 09:48

reset snakemake default

a0c87a8

remove unused import

8f69645

Merge remote-tracking branch 'origin/main' into linopy-dev

83b4cca

GbotemiB force-pushed the linopy-dev branch from bae6709 to 83b4cca Compare July 18, 2024 02:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linopy Integration #351

Linopy Integration #351

GbotemiB commented Jun 24, 2024 •

edited

Loading

ekatef commented Jun 27, 2024

GbotemiB commented Jun 27, 2024

ekatef commented Jun 27, 2024 •

edited

Loading

ekatef Jun 27, 2024 •

edited

Loading

GbotemiB Jun 27, 2024

ekatef Jul 3, 2024

GbotemiB Jul 12, 2024

ekatef commented Jun 27, 2024

GbotemiB commented Jun 27, 2024

davide-f left a comment

davide-f Jul 1, 2024

GbotemiB Jul 2, 2024

davide-f Jul 2, 2024

GbotemiB Jul 2, 2024

ekatef Jul 3, 2024

GbotemiB Jul 3, 2024

ekatef Jul 3, 2024

GbotemiB Jul 3, 2024 •

edited

Loading

ekatef commented Jul 3, 2024

GbotemiB commented Jul 3, 2024

GbotemiB commented Jul 3, 2024

GbotemiB commented Jul 12, 2024

davide-f left a comment

davide-f Jul 16, 2024

davide-f Jul 16, 2024

GbotemiB Jul 18, 2024

davide-f Jul 16, 2024

GbotemiB commented Jul 18, 2024

davide-f commented Jul 18, 2024

GbotemiB commented Jul 18, 2024

		if co2_stores.empty or ("Store", "e") not in n.variables.index:
		if co2_stores.empty: # or ("Store", "e") not in n.variables.index:


		mem: 30000 #memory in MB; 20 GB enough for 50+B+I+H2; 100 GB for 181+B+I+H2
		walltime: "12:00:00"

		vars_final_co2_stored = get_var(n, "Store", "e").loc[sns[-1], co2_stores]
		vars_final_co2_stored = n.model["Store-e"].loc[sns[-1], co2_stores]

		lhs = linexpr((1, vars_final_co2_stored)).sum()
		lhs = (1 * vars_final_co2_stored).sum()

		clustered_pop_layout="resources/population_shares/pop_layout_elec_s{simpl}_{clusters}_{planning_horizons}.csv",
		clustered_pop_layout="resources/population_shares/pop_layout_elec_s{simpl}_{clusters}.csv",

		h2_network_cap = n.model["Link-p_nom"]
		lhs = (h2_network_cap.loc[h2_network.index] * h2_network.length).sum()

Linopy Integration #351

Are you sure you want to change the base?

Linopy Integration #351

Conversation

GbotemiB commented Jun 24, 2024 • edited Loading

Closes # (if applicable).

Changes proposed in this Pull Request

Checklist

ekatef commented Jun 27, 2024

GbotemiB commented Jun 27, 2024

ekatef commented Jun 27, 2024 • edited Loading

ekatef Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekatef commented Jun 27, 2024

GbotemiB commented Jun 27, 2024

davide-f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GbotemiB Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

ekatef commented Jul 3, 2024

GbotemiB commented Jul 3, 2024

GbotemiB commented Jul 3, 2024

GbotemiB commented Jul 12, 2024

davide-f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GbotemiB commented Jul 18, 2024

davide-f commented Jul 18, 2024

GbotemiB commented Jul 18, 2024

GbotemiB commented Jun 24, 2024 •

edited

Loading

ekatef commented Jun 27, 2024 •

edited

Loading

ekatef Jun 27, 2024 •

edited

Loading

GbotemiB Jul 3, 2024 •

edited

Loading