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

Feature/sustainable solution for bug #468

Merged
merged 14 commits into from Jan 16, 2024

Conversation

mgovers
Copy link
Member

@mgovers mgovers commented Jan 15, 2024

Sustainable solution for #461 .
Replaces quick fix in #462 .
Tests in #460 .

A large portion of the complexity in the batch update is refactored away to separate lambda functions:

  • The construction of the sub batch calculation is moved to a separate function, as well as its dispatching
  • Each scenario itself has a separate setup, winddown and run and also has an exception handler and a recovery method for otherwise unfixable errors
  • the Lippincott pattern is used to handle all scenario exceptions in one go https://en.cppreference.com/w/cpp/error/current_exception . That way, the actual implementation for the running of the scenario only requires one try/catch block

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers added the improvement Improvement on internal implementation label Jan 15, 2024
@mgovers mgovers self-assigned this Jan 15, 2024
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@TonyXiang8787
Copy link
Member

should this be merged before #460?

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
Copy link
Member Author

mgovers commented Jan 15, 2024

should this be merged before #460?

you mean #467 ? #460 was already merged

i would argue for merging #467 first. That isolates the repro case from implementation work (TDD)

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Base automatically changed from feature/tests-for-restore-bug to main January 15, 2024 13:26
mgovers and others added 5 commits January 15, 2024 14:27
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@TonyXiang8787
Copy link
Member

@mgovers If this is still working in progress, please put the PR in draft.

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers
Copy link
Member Author

mgovers commented Jan 16, 2024

@mgovers If this is still working in progress, please put the PR in draft.

everything is done except for sonar cloud/CI. i thought it was already ready for review until i saw the code smells. CI should pass now. If not, i'll pull it back to draft

@mgovers
Copy link
Member Author

mgovers commented Jan 16, 2024

remaining code smells were already introduced in original main but just were moved around

Copy link
Member

@TonyXiang8787 TonyXiang8787 left a comment

Choose a reason for hiding this comment

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

We were using some confusing names before. Since we are refactoring here, please change the names of variables correctly using the terminology defined here.

https://power-grid-model.readthedocs.io/en/stable/user_manual/dataset-terminology.html

Especially, we use the loop variable name batch_number , it should be actually scenario_number. We only have one batch, which consists of multiple sub-batches for multithreading, which consist again multiple scenarios per sub-batch.

@mgovers mgovers marked this pull request as draft January 16, 2024 09:45
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers marked this pull request as ready for review January 16, 2024 09:54
Copy link

sonarcloud bot commented Jan 16, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

3 New issues
0 Security Hotspots
97.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@mgovers mgovers merged commit 584f047 into main Jan 16, 2024
26 checks passed
@mgovers mgovers deleted the feature/sustainable-solution-for-bug branch January 16, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement on internal implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants