[BUG] calling `attach_ids` on an array blocks `append` of that array afterwards since 1.4.7

[vincentkoppen]

Describe the bug

Before v1.4.7 we could do the following

from power_grid_model_ds import Grid
from power_grid_model_ds.arrays import NodeArray

grid = Grid.empty()
nodes = NodeArray.empty(2)
grid.attach_ids(nodes)
grid.append(nodes)

But after the merge of #92 this raises an error:
ValueError: Cannot append: minimum id 1 is not greater than the current id counter 2

Cause
The attach_ids already updates the internal counter, this is sensible on the one hand since this allows you to do something like:

grid = Grid.empty()
nodes = NodeArray.empty(1)
lines = LineArray.empty(1)
grid.attach_ids(nodes)
grid.attach_ids(lines)

where the node will have id 1 and the line will have id 2.
However it blocks the example given above.

Solution
The change in 1.4.7 was a breaking change for our code, since we used the flow as described.
Personally I view the described flow that now breaks as a valid one. We use it so that we know what the id will be of a certain index of the array that we add to the grid. This seems like a useful thing to have.

The solutions I see for now:

1. If you want the flow described, you need to set check_max_ids to False (our current fix).
2. We do no longer update the internal counter within attach_ids. This breaks the flow in the Cause section.
3. The append returns a list of ids that match their index. Removing the need for attach_ids

[jaapschoutenalliander]

As I see it, this exposes a danger in having a public attach_ids method. If we go with option 3, we can potentially deprecate this but maybe there are some objections to this? Now having an object outside the grid relate to the internal counter seems counter-intuitive (pun-intended).

A pragmatic solution indeed is option 1, which I would now normally go for in implementation. I am not a big opponent of option 2 though, I am curious to how others use this functionality @Thijss?

[Thijss]

We currently use this functionality in our simulation for example in the following case:

(pseudo code)

def connect_new_client(grid: Grid, from_node: int):
    # This client requires an extra line & node to connect to an existing node.
    
    new_node = NodeArray.empty(1)
    grid.attacht_ids(new_node)
    new_line = LineArray.empty(1)
    new_line.from_node = from_node
    new_line.to_node = new_node.id
    
    # more logic concerning new client
    ...



    # finally, add all info to the grid
    grid.append(new_node)
    grid.append(new_line)
    ...

The reason we prefer attach_ids here is that no records are added to the grid if the code somehow fails halfway collecting all info for a client. Only the id_counter is updated a bit, which is a non-issue.

I believe we use check_max_ids=False too, otherwise I'd expect it to faill as @vincentkoppen described.

---

I think for now I'd also prefer the pragmatic solution (option 1), as I see attach_ids as a more advanced method. I guess I kinda expect .attach_id users to know what they're doing. We could maybe extend the attach_id docstring to give more examples on how to deal with these cases?

[vincentkoppen]

I believe we use check_max_ids=False too, otherwise I'd expect it to faill as @vincentkoppen described.
You might see a failure if you don't if you update to the latests PGM-DS.

I agree that this mainly showd the risk of the public attach_ids that both works internally (counter of the grid) and externally (the array). I do feel that for now the pragmatic of just using check_max_ids=False is definitely workable.

[vincentkoppen]

For the 3rd option I noticed some unexpected behaviour which it something we problably think about. The array that is given to grid.append is updated by reference, so the ids of the original array are set after the append call, which is probably not expected by the user.

grid = Grid.empty()
node = NodeArray.empty(1)
print(node)
grid.append(node)
print(node)

Output:

id     | u_rated | node_type | feeder_branch_id | feeder_node_id | component_id | substation_type | simultaneity 
 -2147483648 |   nan   |     0     |   -2147483648    |  -2147483648   | -2147483648  |        -1       |     nan     
 
 id | u_rated | node_type | feeder_branch_id | feeder_node_id | component_id | substation_type | simultaneity 
 1  |   nan   |     0     |   -2147483648    |  -2147483648   | -2147483648  |        -1       |     nan      

[Thijss]

Another option I thought of:

rename attach_ids to reserve_ids so it's clear what happens?