Creating classifications and checking equality could be confusing

[petebankhead]

When looking at #44 I noticed that the behavior of Classification is different from what I’d expect.

In QuPath, PathClass should be a singleton and so you need to get it from PathClass.fromString(String) or similar. QuPath’s PathClass has a private constructor and tries to make it very hard to use accidentally (e.g. from Groovy).

Here, Classification.get_cached_classification() has that role - but if I’m reading it right, it’s still very possible to use the constructor. Additionally, there isn’t a check to make sure that names really is a tuple.

def __init__(
        self,
        names: Union[str, tuple[str]],
        color: Optional[tuple[int, int, int]] = None,
    ):
        """
        :param names: the names of the classification
        :param color: the RGB color (each component between 0 and 255) of the classification. Can be None to use a random color
        """
        if isinstance(names, str):
            names = (names,)
        self._names = names
        self._color = (
            tuple(random.randint(0, 255) for _ in range(3)) if color is None else color
        )

Also, the equality test is currently

def __eq__(self, other):
        if isinstance(other, Classification):
            return self.name == other.name and self.color == other.color
        return False

If Classification is used properly, then I think an is test should come first for efficiency: we’d expect the same classification to be the same object, and we don’t want the overhead of string joining every time (although that could be mitigated if we cached the joined string as well).

But also, this seems to suggest that a classification of (for example) Tumor might differ from another classification of Tumor if they have different colors. This shouldn’t be possible in QuPath, because the constructor can’t be used - but if it were possible, the color shouldn’t matter for equality tests.

Ultimately, the behavior aims to make sure that each classification can only have one color. When requesting a classification, the user might specify a color - but that is only used if a classification with the same name doesn’t already exist. If it does exist, then the existing classification should always be returned.

[alanocallaghan]

Private constructors don't really exist and aren't common in Python, see for example https://stackoverflow.com/questions/8212053/private-constructor-in-python

I'm a bit wary about using hacks to get around this and would be more inclined to separate color from classification, perhaps getting class color from a cache of dict[str, color]

There are certainly insufficient checks.

[alanocallaghan]

Continuing the conversation from #44...

Quoting some considerations and tradeoffs:

2. Equality testing and ordering (CD3: CD8 vs. CD8: CD3)

I don't really understand why equality testing is linked with ordering?
The use of a tuple ensures ordering is maintained.

3. Uniqueness of classification names (CD3: CD3 possible?)

Yes it is possible; I'm not sure if we ought to disallow it.
It is also possible in QuPath, and I'm inclined to let people shoot themselves in the foot if they want to.

4. Creating a convenient string representation (if this is even necessary?)

• : may have been a bad choice originally, see Class label entry adds space after a colon qupath#507

I can't say I'm a fan of using a string as a data structure for input/storage, and would much rather store parts and supply some sort of toString equivalent.

6. Linking colors with classifications consistently
7. Access to constructors or use of singletons for efficiency

• QuPath enforces singletons partly because creating 1,000,000 objects or 1,000,000 cells would have a non-trivial overhead

I think 7 follows easily from 6.
The cache suggests we want to do this, but the rest of the code isn't cooperating.
Practically, I think this means removing the "get_cached" method and overriding __new__ to check the cache before returning a new instance.

8. Similarity to QuPath's API
9. Feels sensible in Python... not weirdly-ported Java

Unfortunately I think these two are hard to reconcile. Java code is not Pythonic, and vice versa.