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

Add vulnerability functions. #326

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

xbarra
Copy link
Collaborator

@xbarra xbarra commented Aug 2, 2024

We add vulnerability functions with the aim of replicating the paper:
'Climate change-related statistical indicators'
Statistics Paper Series No 48 from the European Central Bank.
https://www.ecb.europa.eu/pub/pdf/scpsps/ecb.sps48~e3fd21dd5a.en.pdf?4826f6ee77d3ac7a681916b6d419b751

Co-authored-by: Virginia Morales [email protected]
Co-authored-by: Víctor de Luna [email protected]
Co-authored-by: Arfima Dev [email protected]

Signed-off-by: Virginia Morales [email protected]

@xbarra xbarra requested a review from joemoorhouse as a code owner August 2, 2024 15:48
@xbarra xbarra requested review from joemoorhouse and removed request for joemoorhouse August 2, 2024 15:48
@xbarra xbarra marked this pull request as draft August 2, 2024 15:49
@xbarra
Copy link
Collaborator Author

xbarra commented Aug 2, 2024

I am reviewing all the tests before adding the commit, I'll try to have it on Monday and add more descriptions.

We'll do a separate PR for the documentation, ideally next week.

Copy link
Contributor

@ModeSevenIndustrialSolutions ModeSevenIndustrialSolutions left a comment

Choose a reason for hiding this comment

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

Looks good to me.
(Presumably you'll sort the pre-commit/linting checks when this comes out of draft?)

@xbarra
Copy link
Collaborator Author

xbarra commented Aug 5, 2024

Looks good to me. (Presumably you'll sort the pre-commit/linting checks when this comes out of draft?)

yes, want to pass cleanly

@joemoorhouse
Copy link
Collaborator

Ah, looks like this has some conflicts with the just-merged #310 unfortunately. @xbarra, I can take a look at that if you like unless you have already resolved? - the ground just shifted under your feet somewhat!

Hi @ModeSevenIndustrialSolutions, what's the plan about linting for OSC projects actually - are we meant to using ruff or black (and indeed will they clash?).

@ModeSevenIndustrialSolutions
Copy link
Contributor

Hi @ModeSevenIndustrialSolutions, what's the plan about linting for OSC projects actually - are we meant to using ruff or black (and indeed will they clash?).

Ruff replaces a lot of separate python linting tools, and apart from getting the benefit of consolidation, is much faster and more efficient. It also de-centralises the configuration options because it can read its config from stanza in pyproject.toml. Hopefully we can all therefore perhaps agree on the value of this tool, and mandate its use, but configuration can be modified on a per-repo basis. Therefore, we don't need to maintain some central "one size doesn't fit all" configuration that will be unpalatable to lots of people.

I've been working on a whole bunch of DevOps tooling updates, and will hopefully be able to demonstrate the progress on this topic in a couple of weeks time!

  • Matt

@xbarra
Copy link
Collaborator Author

xbarra commented Aug 7, 2024

@joemoorhouse I have already resolved them.

I am also going to push the change to ruff as linter

VMarfima and others added 2 commits August 7, 2024 08:45
We add vulnerability functions with the aim of replicating the paper:
'Climate change-related statistical indicators'
Statistics Paper Series No 48 from the European Central Bank.
https://www.ecb.europa.eu/pub/pdf/scpsps/ecb.sps48~e3fd21dd5a.en.pdf?4826f6ee77d3ac7a681916b6d419b751

Co-authored-by: Víctor de Luna <[email protected]>
Co-authored-by: Arfima Dev <[email protected]>

Signed-off-by: Virginia Morales <[email protected]>
Add tests for UseCaseID.STRESSTEST vulnerability functions.
Ensure all tests pass.

Co-authored-by: Virginia Morales <[email protected]>
Co-authored-by: Arfima Dev <[email protected]>
Co-authored-by: Xavier Barrachina Civera <[email protected]>

Signed-off-by: Víctor de Luna <[email protected]>
pyproject.toml Outdated Show resolved Hide resolved
@xbarra
Copy link
Collaborator Author

xbarra commented Aug 7, 2024

We are fixing the mypy complains.

I will add mypy to the pre-commit hooks config.

@xbarra xbarra force-pushed the stress_test branch 2 times, most recently from 174c9f6 to b2d4cf4 Compare August 7, 2024 18:00
@xbarra xbarra marked this pull request as ready for review August 7, 2024 18:06
@xbarra xbarra force-pushed the stress_test branch 3 times, most recently from b5c697a to 89221f9 Compare August 8, 2024 13:16
@xbarra
Copy link
Collaborator Author

xbarra commented Aug 8, 2024

Note: This PR's commits should not be squashed together, since they are quite different.

Once again, albeit I am extremely partial to the linear history, merging commits have unintended consequences.

I am more than happy in doing a manual rebase if needed. [Not that I want to have more stuff to do in my life, but for linear history, I would do it].

Also the workflow is using isort, and because of that it fails.

@xbarra xbarra force-pushed the stress_test branch 6 times, most recently from b2f5bf6 to 104bc2b Compare August 8, 2024 16:59
@joemoorhouse
Copy link
Collaborator

Hi @xbarra, thanks for all of the fixes, the ruff and test commits are looking good I think. On the vulnerability changes, I am a bit confused by the StressTestImpact - I don't understand why this is added into the kernel (i.e. this is really changing the framework) alongside the existing ImpactDistrib class. As far as I can tell, this contains an instance of HazardPercentilesStressTest which is very specific to the ECB stress-testing methodology and contains specific values relating to that approach (and indeed is in risk_models module not in the kernel which is the right place for it). In other words we have an element added to the kernel which relates to one specific methodology I think.

I think what is going on is that you have some RiskMeasureCalculators e.g. ThermalPowerPlantsRiskMeasures that is comparing to a baseline which is actually a hard-coded set of parameters, but then I would have thought that HazardPercentilesStressTest that contains those parameters should simply be in that class? In other words, base_impact is not actually being used in this case, which is fine, but I don't think that should lead to kernel/framework changes.

Happy to discuss. This is actually a largish framework change, impacting the kernel (in particular risk.py) so I think it's important to check this point.

Thanks,
Joe

@xbarra
Copy link
Collaborator Author

xbarra commented Aug 9, 2024

Hi @xbarra, thanks for all of the fixes, the ruff and test commits are looking good I think. On the vulnerability changes, I am a bit confused by the StressTestImpact - I don't understand why this is added into the kernel (i.e. this is really changing the framework) alongside the existing ImpactDistrib class. As far as I can tell, this contains an instance of HazardPercentilesStressTest which is very specific to the ECB stress-testing methodology and contains specific values relating to that approach (and indeed is in risk_models module not in the kernel which is the right place for it). In other words we have an element added to the kernel which relates to one specific methodology I think.

I think what is going on is that you have some RiskMeasureCalculators e.g. ThermalPowerPlantsRiskMeasures that is comparing to a baseline which is actually a hard-coded set of parameters, but then I would have thought that HazardPercentilesStressTest that contains those parameters should simply be in that class? In other words, base_impact is not actually being used in this case, which is fine, but I don't think that should lead to kernel/framework changes.

Happy to discuss. This is actually a largish framework change, impacting the kernel (in particular risk.py) so I think it's important to check this point.

Thanks, Joe

Thank you so much for the feedback, Joe.
We are adjusting our changes to that.

@xbarra
Copy link
Collaborator Author

xbarra commented Aug 9, 2024

Hi @joemoorhouse
the last revision has your proposed ajustments.

Let us know of other changes needed.

@xbarra
Copy link
Collaborator Author

xbarra commented Aug 9, 2024

Moved back use_case_id to a string.

@xbarra xbarra marked this pull request as draft August 14, 2024 15:48
@xbarra xbarra force-pushed the stress_test branch 2 times, most recently from 0114aaf to f468ecd Compare August 16, 2024 11:29
@xbarra xbarra changed the title Stress test Add vulnerability functions. Aug 16, 2024
@xbarra
Copy link
Collaborator Author

xbarra commented Aug 16, 2024

depends on os-climate/hazard#108
After which, the inventory.json will need to be updated.

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.

5 participants