Skip to content

Conversation

mgovers
Copy link
Member

@mgovers mgovers commented Aug 20, 2025

The location where the calculation info was reset is quite opaque. To more explicitly show the calculation

NOTE: this does not solve the actual issue that main model owns its own info, nor the fact that the location where the reset is done is somewhere halfway through the calculation. It merely makes it more visible what happens under the hood.

Relates to #1082 (reinstigated the original logic to allow benchmarking against original main)
Relates to #1093 (this PR is an enabler for follow-up improvements similar to #1093, but it is not a blocker for it)

Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers self-assigned this Aug 20, 2025
@mgovers mgovers added the improvement Improvement on internal implementation label Aug 20, 2025
nitbharambe
nitbharambe previously approved these changes Aug 20, 2025
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers added the do-not-merge This should not be merged label Aug 20, 2025
@mgovers
Copy link
Member Author

mgovers commented Aug 20, 2025

still something weird going on with the benchmark. do-not-merge label added

Copy link

@mgovers
Copy link
Member Author

mgovers commented Aug 21, 2025

still something weird going on with the benchmark. do-not-merge label added

Found the bug on main that caused the new results to (incorrectly) appear much slower compared to the original one:
image

However, after digging into this, it turns out that the tap position optimizer contains a bug in the following line: https://github.com/PowerGridModel/power-grid-model/blob/feature/migrate-reset-calculation-info/power_grid_model_c/power_grid_model/include/power_grid_model/optimizer/tap_position_optimizer.hpp#L1033

On current main, the calculation info was cleaned up before every tap position optimizer subcalculation, and, as a result, the tap position optimizer was not fully benchmarked, because the intermediate timings were discarded - not great. Moving cleaning responsibility to the JobDispatch meant that we now DO benchmark the tap position optimizer, effectively fixing this bug.

NOTE: this also relates to #1082 .

TBD: we have 3 paths going forward:

@mgovers
Copy link
Member Author

mgovers commented Aug 21, 2025

Decision: merge this PR as-is and recalibrate our benchmark.

@mgovers mgovers removed the do-not-merge This should not be merged label Aug 21, 2025
@mgovers
Copy link
Member Author

mgovers commented Aug 21, 2025

still something weird going on with the benchmark. do-not-merge label added

do-not-merge label removed cfr. above

@mgovers mgovers requested a review from nitbharambe August 21, 2025 07:24
@mgovers mgovers added this pull request to the merge queue Aug 21, 2025
Merged via the queue into main with commit 2c9ebd6 Aug 21, 2025
32 of 33 checks passed
@mgovers mgovers deleted the feature/migrate-reset-calculation-info branch August 21, 2025 08:43
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.

2 participants