Skip to content
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

Fix pre-commit and revise style #779

Merged
merged 3 commits into from
Jun 30, 2023

Conversation

davide-f
Copy link
Member

@davide-f davide-f commented Jun 30, 2023

Changes proposed in this Pull Request

Fix to pre-commit.
The pre-commit has also been partially aligned to the one by pypsa-eur

I feel that this may conflict with other PRs, including #701
#701 shall be first reviewed and merged before fixing this.

Everything can be done in few hours

@davide-f davide-f marked this pull request as draft June 30, 2023 00:08
@davide-f davide-f changed the title Fix pre-commit and revise style [READY BUT NOT MERGE YET] Fix pre-commit and revise style Jun 30, 2023
@pz-max pz-max marked this pull request as ready for review June 30, 2023 06:50
@@ -337,8 +337,8 @@ monte_carlo:
# Key: add below the pypsa object for the monte_carlo sampling, "network" is only allowed for filtering!
# Value: currently supported format [l_bound, u_bound] or empty [], represent multiplication factors for the object
loads_t.p_set: [0.9, 1.1]
generators_t.p_max_pu.loc[:, n.generators.carrier == "wind"]: [0.9, 1.1]
generators_t.p_max_pu.loc[:, n.generators.carrier == "solar"]: [0.9, 1.1]
# generators_t.p_max_pu.loc[:, n.generators.carrier == "wind"]: [0.9, 1.1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davide-f we need at least two rows to test the Monte-Carlo stuff. Test is currently failing because of that

Copy link
Member

@pz-max pz-max Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a commit to fix this... let's see

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recreated the PR and kept this line commented out.
pretty YAML crashes with checking those lines. In this PR, there is a fix to monte_carlo caospy to make sure that it works even with a single parameter in the list of monte carlos. The other options have been tested as well.

@pz-max
Copy link
Member

pz-max commented Jun 30, 2023

pre-commit is failing with exit-code: 3:
The documentation says:

Possible exit codes from docformatter:

    1 - if any error encountered
    2 - if it was interrupted
    3 - if any file needs to be formatted (in --check or --in-place mode)

I think because of the recent PR that we merged, @davide-f you need to run locally the pre-commit and add the fixes that the docformatter is suggesting. Can you do that?

@davide-f
Copy link
Member Author

davide-f commented Jun 30, 2023

if any file needs to be formatted (in --check or --in-place mode)

@pz-max the yaml block of the pre-commit is failing because it cannot handle "generators_t.p_max_pu.loc[:, n.generators.carrier == "wind"]: [0.9, 1.1]"

Therefore, I commented these two lines in the tutorial and default configs.

That lead instead to issues in the CI.
I think two approaches may apply:

  1. fix the monte carlo so to be able to work without those 2 lines, this is I think the best way, but it means that those lines are not tested in the workflow
  2. revise the monte-carlo config test, in the test folder, to add those two lines and ignore that file when doing the pre-commit CI, as otherwise it leads to issues

Since @pz-max, you worked in the monte carlo, what is your feeling there?

@davide-f
Copy link
Member Author

@pz-max The PR has been rewritten and force-pushed with the following changes:

  1. The lines related to generators_t.p_max_pu.loc .... in monte_carlo have been commented out as pretty-yaml crashed in analysing them
  2. The monte_carlo function has been revised to make sure that the function works with all the montecarlo generating options even when a single uncertain parameter is handled, in this case being the load demand

@davide-f davide-f changed the title [READY BUT NOT MERGE YET] Fix pre-commit and revise style Fix pre-commit and revise style Jun 30, 2023
@davide-f
Copy link
Member Author

davide-f commented Jun 30, 2023

@pz-max PEP 257 is there, if CI passes this PR can be merged. Locally seems ok

@davide-f davide-f enabled auto-merge (squash) June 30, 2023 19:36
@davide-f davide-f disabled auto-merge June 30, 2023 21:53
@davide-f davide-f merged commit d0d709d into pypsa-meets-earth:main Jun 30, 2023
@davide-f davide-f deleted the fix-pre-commit branch October 1, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants