store max_id to prevent having to recalculate

[JohanMeppelinkDRA]

Fixes #issue_number

Changes proposed in this PR includes

Add _max_id as a parameter to the FancyArrayContainer. The ids area already kind of 'stale' in the sense that they're updated only if people use the container correctly. If they change ids on the arrays directly it could still deviate. This max_id is the same. Max_id is now updated as a single int instead of requiring the max(self._ids) calculation. It sounds bizar but due to calling attach_ids a lot this became a major bottleneck in our code. Making it an int we track will help improve performance significantly for us.

Could you please pay extra attention to the points below when reviewing the PR

If self._ids is updated outside of the FancyArrayContainer (which I think it shouldn't as it's a private variable) that could result in container.max_id to be different than before potentially breaking code. Is this acceptable?

Checks

• [ nvt ] If changes are related to CI/CD, are they verified via a manual run on this branch?
• [ no ] Do you wish to discuss this PR in the bi monthly community meeting?
  Please join the PGM community meeting

[vincentkoppen]

Hi @JohanMeppelinkDRA

Thanks for the PR and raising the issue!
This has been an oversight on our part when we introduced the ids property, we thought taking the max would be quick but this indeed isn't at all. So definitely something we need to fix.

The solution in this PR seems to work as intended, however we also feel that it introduces some risk for future maintenance.
With the setup we always need to remember ourselves that any time we touch _ids, we also need to update _max_ids. Which might introduce bugs later on.

Instead we propose a solution where we tie this logic together in one spot, where we basically have a wrapper around a set that keeps track of it's maximum when adding any values.

---

Let me know if you are willing to pick up the changes proposed below.
Otherwise I can also help/pick it up further if needed.
For future issues, you can always first make an issue so we can discuss the solution direction before implementation.

---

Porposed solution

We could introduce a simple IdTracker, where the add method automatically keeps track of both.

class IdTracker:
    def __init__(self, ids: set | None = None) -> None:
        self._ids = ids if ids is not None else set()
        self._max_id = max(self._ids) if self._ids else 0

    @property 
    def ids(self) -> set[int]: 
        return self._ids

    @property
    def max_id(self) -> int: 
        return self._max_id
  
    # The max_new_id allows you to give the max_id of the new_ids directly if known, so skip the max(new_ids).
    def add(self, new_ids: set, max_new_id: int | None = None) -> None: 
        self._ids |= new_ids
        if max_id is not None:
            self._max_id = max(self._max_id, max_id)
        elif new_ids:
            self._max_id = max(self._max_id, max(new_ids))


    def __eq__ (self, other: object) -> bool:
        if not isinstance(other, self.__class__):
            return False
        return self._ids == other._ids and self._max_id == other._max_id

Within the Grid we now just have:

• _ids: set -> _id_tracker: IdTracker or a better name
• Any interaction with self._ids now becomes self._id_tracker.add(...)
• The properties ids and max_id reference the matching _id_tracker properties.

[JohanMeppelinkDRA]

aangepast naar de gewenste IdTracker. Maakt het inderdaad minder fout gevoelig.

[jaapschoutenalliander]

The setup and testing looks good, thanks for making the effort. CI is failing on some minor details (docstrings)

[jaapschoutenalliander]

Linter is failing on missing docstrings here and in the other functions of the IdTracker