pypsa-meets-earth / pypsa-earth-sec

GNU General Public License v3.0

20 stars 17 forks source link

feat: modify copy commit processing rule #237

Closed doneachh closed 10 months ago

doneachh commented 1 year ago

Closes # (if applicable).

Changes proposed in this Pull Request

Added a copy commit function, which stores the commit id and commit message of pypsa earth sec and the submodule in the n.meta of the .nc file

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.

doneachh commented 1 year ago

@energyLS please review! :)

doneachh commented 1 year ago

Hey @davide-f :) implemented the copy commit feature for pypsa-earth sec as well. Can you review it here as well? 🙏

doneachh commented 11 months ago

@davide-f tried to implement the review aspects like in pypsa-earth: Somehow it doesnt work here. As soon as we import scripts.helpers in the Snakefile of pypsa-earth sec we get an error that we dont find the module scripts._helpers in the Snakefile of pypsa earth. I dont understand why this is happening. Do you have an idea? :)

davide-f commented 11 months ago

As additional comment, in PyPSA-Earth we do have:

import sys
sys.path.append("./scripts")

May be good to check if that's necessary to have in pypsa-earth and whether it's absence in -sec may lead to issues

Unfortunately these weird behaviours are most likely due to the subworkflows that are not well maintained...

doneachh commented 10 months ago

Hi @davide-f, sorry for my late response: Checked

import sys
sys.path.append("./scripts")

adding it in the snakefile of pypsa-earth-sec, which didnt work. Also i tried to remove it in the snakefile of pypsa-earth, which didnt work as well. Do you have another idea of how to fix it? Otherwise i would suggest to copy the commit not already in the beginning of the snakefile and copy it in the solve_network function - this would work out :)

davide-f commented 10 months ago

Hello! :D

So, I've locally tested your PR and I made it run by removing the "scripts." in the pypsa-earth Snakefile in the imports like "from scripts._helpers". Could you investigate if that works also for the standard pypsa-earth? Once verified, you could create a PR in pypsa-earth where we revise the setup of the get_commit function and also revise the snakefile as done here.

I think we are close to finalize :)

doneachh commented 10 months ago

Hi @davide-f :) the removement of "scripts." works! 👍 Thanks for your help - how do we continue with this PR as it only works with the modified submodule?

doneachh commented 10 months ago

Hi @davide-f :D Merged main into the branch and updated the submodule! Thank you so much for your help :)

davide-f commented 10 months ago

super :) CI is also passing. I think this PR can be merged

energyLS commented 10 months ago

super :) CI is also passing. I think this PR can be merged

@davide-f then I will merge it? :)

davide-f commented 10 months ago

super :) CI is also passing. I think this PR can be merged

@davide-f then I will merge it? :)

Yeah :D