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

Partial Failure and completeData #1256

Merged
merged 14 commits into from
Feb 10, 2025
Merged

Partial Failure and completeData #1256

merged 14 commits into from
Feb 10, 2025

Conversation

RoxaneChen02
Copy link
Collaborator

@RoxaneChen02 RoxaneChen02 commented Jan 7, 2025

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Does this PR already have an issue describing the problem?

Fixes #1238

What kind of change does this PR introduce?

What is the current behavior?

  • systematicSensitivityResult status of the initial 2P perimeter is set to FAILURE if sensi analysis on state with applied RAs failed even if precontingency sensi are successes, which is not coherent with the behavior during the other instants.
  • use completeDataWithFailingPerimeter to handle failure during 2P initial sensi

What is the new behavior (if this is a feature change)?

  • systematicSensitivityResult status of the initial 2P perimeter is set to PARTIAL_FAILURE if sensi analysis status on state with applied RAs failed.
  • remove completeDataWithFailingPerimeter to reuse preexisting completeData function

Does this PR introduce a breaking change or deprecate an API?

  • Yes
  • No

If yes, please check if the following requirements are fulfilled

  • The Breaking Change or Deprecated label has been added
  • The migration steps are described in the following section

What changes might users need to make in their application due to this PR? (migration steps)

Other information:

CHEN Roxane added 3 commits December 26, 2024 16:47
@RoxaneChen02 RoxaneChen02 requested review from pet-mit, Godelaine and phiedw and removed request for pet-mit January 7, 2025 16:32
Signed-off-by: CHEN Roxane <[email protected]>
pet-mit
pet-mit previously requested changes Jan 7, 2025
Copy link
Collaborator

@pet-mit pet-mit left a comment

Choose a reason for hiding this comment

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

I'm not sure this fixes the case where N succeeds and all N-1 fail (should give partial failure, currently gives failure, because of line 84 of SystematicSensitivityResult

@RoxaneChen02 RoxaneChen02 removed the request for review from phiedw January 7, 2025 16:52
@RoxaneChen02
Copy link
Collaborator Author

For me this case is handled by the line 89. If N succeeds this.status is set to success after line 89 and and keeps this value even if all N-1 fail. So in the end we have anyContingencyFailure = true and nStateResult.status = SUCCESS -> this .status is set to PARTIAL_FAILURE as it should ?

@pet-mit
Copy link
Collaborator

pet-mit commented Jan 8, 2025

For me this case is handled by the line 89. If N succeeds this.status is set to success after line 89 and and keeps this value even if all N-1 fail. So in the end we have anyContingencyFailure = true and nStateResult.status = SUCCESS -> this .status is set to PARTIAL_FAILURE as it should ?

can you try to add a unit test to test it out ?

CHEN Roxane added 2 commits January 8, 2025 17:09
Signed-off-by: CHEN Roxane <[email protected]>
@RoxaneChen02 RoxaneChen02 changed the title Refacto partial failure Partial Failure and completeData Jan 13, 2025
Signed-off-by: CHEN Roxane <[email protected]>
@RoxaneChen02
Copy link
Collaborator Author

For me this case is handled by the line 89. If N succeeds this.status is set to success after line 89 and and keeps this value even if all N-1 fail. So in the end we have anyContingencyFailure = true and nStateResult.status = SUCCESS -> this .status is set to PARTIAL_FAILURE as it should ?

can you try to add a unit test to test it out ?

Case already tested in testPartialContingencyFailures() in SystematicSensitivityResultTest.

@RoxaneChen02 RoxaneChen02 requested review from phiedw and removed request for Godelaine January 14, 2025 10:06
CHEN Roxane added 2 commits January 14, 2025 11:59
Signed-off-by: CHEN Roxane <[email protected]>
Signed-off-by: CHEN Roxane <[email protected]>
@Godelaine Godelaine added the PR: waiting-for-review This PR is waiting to be reviewed label Jan 17, 2025
@RoxaneChen02
Copy link
Collaborator Author

I'm not sure this fixes the case where N succeeds and all N-1 fail (should give partial failure, currently gives failure, because of line 84 of SystematicSensitivityResult

I'm not sure this fixes the case where N succeeds and all N-1 fail (should give partial failure, currently gives failure, because of line 84 of SystematicSensitivityResult

done

@RoxaneChen02 RoxaneChen02 dismissed pet-mit’s stale review January 20, 2025 12:27

case already tested

@Godelaine Godelaine merged commit fa3935f into main Feb 10, 2025
12 checks passed
@Godelaine Godelaine deleted the refacto_partial_failure branch February 10, 2025 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: waiting-for-review This PR is waiting to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Partial Failure and completeData
4 participants