-
Notifications
You must be signed in to change notification settings - Fork 43
Clean-up main model: migrate reset calculation info to job dispatch #1094
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
Conversation
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
power_grid_model_c/power_grid_model/include/power_grid_model/job_interface.hpp
Outdated
Show resolved
Hide resolved
power_grid_model_c/power_grid_model/include/power_grid_model/job_interface.hpp
Outdated
Show resolved
Hide resolved
still something weird going on with the benchmark. do-not-merge label added |
|
Found the bug on 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 NOTE: this also relates to #1082 . TBD: we have 3 paths going forward:
|
Decision: merge this PR as-is and recalibrate our benchmark. |
do-not-merge label removed cfr. above |
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)