Refactor classification code to handle multiple classifications #44
Conversation
Previously the assumption was a classification has one name.
However QuPath features can have multiple classifications:
an object with class "A: B: C" is converted to JSON something like
{[blah], "properties": {"classification": {"names": ["A", "B", "C"]}}}
whereas an object with class "A" is converted something like
{[blah], "properties": {"classification": {"name": "A"}}}
This PR removes the assumption of a single class while ensuring a single
name can be fetched using a mocked `name` property that joins on ": ",
similar to objects with multiple classes in QuPath.
Resolve qupath#43
|
Can you add code examples to the PR for how this should look? I think I like the ideal that From the description, I think I mention it because I have some regrets at introducing both Also, it seems counterintuitive to me that However the main thing I’d like is a clear example in the first post showing how the Python code looks. The description is a bit hard to parse, since it’s not clear if the changes are made only for JSON purposes. |
I'm not sure what I would be demonstrating?
I would tend to treat these as effectively immutable anyway.
It is admittedly not the best time to try parsing this but I don't understand what you mean.
Not entirely; previously we did not really handle multiple classifications in any meaningful way. Technically I suppose you could have by manually manipulating parts of the hierarchical name (without parsing them from qupath) |
Example of intended use and behavior: classification = Classification.get_cached_classification(('Tumor', 'Positive'))
classification.names # ('Tumor', 'Positive')
classification.name # 'Tumor: Positive'As mentioned at #45 I presume this would 'work', but could be problematic: classification = Classification(('Tumor', 'Positive'))Although writing that has me thinking that the following could be problematic as well: classification = Classification('Tumor: Positive'))
classification = Classification.get_cached_classification('Tumor: Positive')And I wonder what would happen if someone tries classification = Classification(['Tumor', 'Positive'])(I haven't tried running the code, this is only from reading it - maybe there are guards I'm missing, or my Python is too rusty)
Fair. It's the use of I'm not saying we should replicate QuPath's confusing behavior, but maybe an alternative to
I'm referring to the normal use of singular and plural forms. Expected Unexpected |
I was aiming to parallel
This is extremely unintuitive to me to the point that I don't understand why it's implemented this way or why this behaviour is at all desirable. It seems like the API requires further changes, I will write some unit tests, update the documentation, and add example code in a future PR. |
|
Also I would suggest we keep further discussion in one thread? Fine with the issue or here |
Yes. Before going too deep, we need a clear definition of what exactly the code is meant to be doing. There are a lot of issues / tradeoffs to balance:
Sure, we can drop this thread and factor the list above into a new discussion on any proposed redesign.
QuPath's approach has (just about) served its purpose for a decade, but I'm not going to argue it's how it ought to have been designed. So this is a chance to do something better. |
3 participants
Previously the assumption was a classification has one name. However QuPath features can have multiple classifications: an object with class "A: B: C" is converted to JSON something like {[blah], "properties": {"classification": {"names": ["A", "B", "C"]}}} whereas an object with class "A" is converted something like {[blah], "properties": {"classification": {"name": "A"}}}
This PR removes the assumption of a single class while ensuring a single name can be fetched using a mocked
nameproperty that joins on ": ", similar to objects with multiple classes in QuPath.Resolve #43