Fix DiscreteShuntController controlled_bus reindexing

[Ehsanpoly]

Fixes #2946.

This updates controller object bus references during bus reindexing. Previously, create_continuous_bus_index() updated element tables such as net.shunt.bus, but DiscreteShuntController.controlled_bus could remain stale.

A regression test was added for DiscreteShuntController after drop_buses() followed by create_continuous_bus_index().

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.64%. Comparing base (412cad8) to head (655a32b).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3065   +/-   ##
========================================
  Coverage    71.63%   71.64%           
========================================
  Files          355      355           
  Lines        39370    39372    +2     
========================================
+ Hits         28204    28207    +3     
+ Misses       11166    11165    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Ehsanpoly]

Thanks. I checked the remaining CI failures.

The relying(python3.10) failure appears to be the same downstream JSON/from_json converter issue seen in other PRs.

The pandapower / warnings(3.14.2) failure occurs during collection of pandapower/test/plotting/test_geo.py, where mv_oberrhein() triggers a DeprecationWarning about missing tap_dependency_table. This path is unrelated to this PR, which only updates bus reindexing for controller object references and adds a regression test for DiscreteShuntController.

The focused regression test passes locally:
python -m pytest pandapower/test/control/test_shunt_control.py -k "discrete_shunt_controlled_bus_reindexed"

[vogt31337]

@Ehsanpoly , I understand your Idea, but I would not like to do it this way. Since your implementation modifies the inner data structure of all controller, if the have a bus attribute. You cannot per se say that this structure will always hold the bus mapping.

In my opinion it would be better to create an interface / put a method in the controller parent object, which takes the job and can be overwritten by controller developer if needed. This way you make sure, not to get any side effects, which can be really hard to track.

Therefore I will reject this PR.

[KS-HTK]

What type of controller operates directly on a bus? Only these controllers should store a bus reference. If a controller is for a shunt or trafo it should always look up the bus it's controlled element is at. And not store the reference. If this lookup has to be done often in a single timeseries execution it could be cached for the run, but on the next run it should be reevaluated.

Since I am rather sure that most controllers are not "controlling" a bus, since there is not much to control there, I would rather suggest just to remove the stored variable all together and replace it with a property that activly gets the current bus from the networks data structure.

[Ehsanpoly]

@vogt31337 @KS-HTK Thank you for the feedback. I agree that my previous implementation was too broad because it modified controller internals generically from reindex_buses().

I will revise the PR by removing the generic controller update from data_modification.py and making the shunt controller resolve its controlled bus from the current net.shunt table instead of storing a stale bus reference.

[vogt31337]

Yeah Sounds better @Ehsanpoly, and we will introduce an Interface for Controller, so If your Controller depends in the voltage of a Bus for Exemplar IT will "Auto" Update.

[Ehsanpoly]

@vogt31337 I revised the PR according to your feedback.

The generic controller update in reindex_buses() has been removed. The shunt controller now resolves the controlled bus dynamically from the current net.shunt table when no explicit bus_index is provided, so create_continuous_bus_index() no longer leaves it stale.

I also updated the regression test to cover drop_buses() followed by create_continuous_bus_index().

Thank you for your hints.

[KS-HTK]

Will this function not have the same Issue?

So on its first call self.bus_index may be None, but on any subsequent call self.bus_index should never be None, as it is never unset.
I would remove the if and have only the net.shunt.at… return. this way it would resolve each time.
But this might be slow.

The other option is to have a property controlled_bus, using a setter, getter and deleter and adding a call to each controllers deleter at the end of a loadflow/timeseries. To only resolve the call once.
@vogt31337 What is you're opinion on this? Will having the two calls to this in the control_step be to costly for a timeseries? Or should we just use the full lookup in those two cases? (or add a local var in the control_step that saves it, reducing it to one lookup in each control step?)

[Ehsanpoly]

@KS-HTK Thank you, I agree. To avoid keeping any stale bus reference, I will simplify controlled_bus() so it always resolves the current bus from net.shunt.

To keep the lookup cost minimal, I will store the result in a local variable inside control_step() and is_converged() instead of calling the lookup repeatedly. I will leave the broader cached property / controller interface idea to a separate design change.

[vogt31337]

Problem is you always do a lookup then. That's why I wanted to have an Interface with an overwritable function.
I think the Bus Array ist for vectorized Access to get Performance Out of it, If you want to Control 10.000 shunts at once.

[Ehsanpoly]

@vogt31337 Thank you, I agree with the performance concern.

Would you prefer that I revise this PR toward the controller-interface approach instead of the dynamic lookup approach?

My understanding is that the clean design would be:

1. Add a default no-op bus-reindex hook to the controller base class, for example reindex_buses(self, bus_lookup).
2. Let reindex_buses(net, bus_lookup, ...) call this hook for controllers, instead of inspecting or mutating arbitrary controller attributes.
3. Override the hook only in ShuntController, where the controller knows exactly which bus reference it owns.
4. Keep the cached controlled_bus for runtime performance, but update it explicitly during bus reindexing.

This would avoid repeated pandas lookups in control_step() / is_converged(), while also avoiding the original broad side effect of mutating every controller with a controlled_bus attribute.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Ehsanpoly]

I updated the implementation according to your comment.

controlled_bus() now always resolves the current bus from net.shunt and no longer uses cached self.bus_index state. I also store the resolved value in a local variable inside control_step() and is_converged() to avoid repeated lookup within the same method call.