BIOP / qupath-extension-abba

QuPath extension for Aligning Big Brains and Atlases
Apache License 2.0
6 stars 2 forks source link

Issue with QuPath v0.4 #11

Closed NicoKiaru closed 1 year ago

NicoKiaru commented 1 year ago

Importing regions without splitting left and right works, but the splitting fails with this error:

INFO: Image data set to ImageData: Fluorescence, Slide_04.vsi - 10x_06
INFO: Loading 257 Atlas Regions for Slide_04.vsi - 10x_06
ERROR: QuPath exception: Cannot invoke "qupath.lib.objects.PathObject.getROI()" because "rootRight" is null
java.lang.NullPointerException: Cannot invoke "qupath.lib.objects.PathObject.getROI()" because "rootRight" is null
    at qupath.ext.biop.abba.AtlasTools.getWarpedAtlasRegions(AtlasTools.java:106)
    at qupath.ext.biop.abba.AtlasTools.loadWarpedAtlasAnnotations(AtlasTools.java:275)
    at qupath.ext.biop.abba.LoadAtlasRoisToQuPathCommand.run(LoadAtlasRoisToQuPathCommand.java:78)
    at qupath.lib.gui.ActionTools.lambda$createAction$1(ActionTools.java:725)
    at org.controlsfx.control.action.Action.handle(Action.java:423)
    at org.controlsfx.control.action.Action.handle(Action.java:64)
    at com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:86)
    at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:234)
    at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:191)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
    at com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:49)
    at javafx.event.Event.fireEvent(Event.java:198)
    at javafx.scene.control.MenuItem.fire(MenuItem.java:459)
    at com.sun.javafx.scene.control.ContextMenuContent$MenuItemContainer.doSelect(ContextMenuContent.java:1385)
    at com.sun.javafx.scene.control.ContextMenuContent$MenuItemContainer.lambda$createChildren$12(ContextMenuContent.java:1338)
    at com.sun.javafx.event.CompositeEventHandler$NormalEventHandlerRecord.handleBubblingEvent(CompositeEventHandler.java:247)
    at com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:80)
    at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:234)
    at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:191)
    at com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
    at com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
    at javafx.event.Event.fireEvent(Event.java:198)
    at javafx.scene.Scene$MouseHandler.process(Scene.java:3894)
    at javafx.scene.Scene.processMouseEvent(Scene.java:1887)
    at javafx.scene.Scene$ScenePeerListener.mouseEvent(Scene.java:2620)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler$MouseEventNotification.run(GlassViewEventHandler.java:411)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler$MouseEventNotification.run(GlassViewEventHandler.java:301)
    at java.base/java.security.AccessController.doPrivileged(Unknown Source)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler.lambda$handleMouseEvent$2(GlassViewEventHandler.java:450)
    at com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:424)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler.handleMouseEvent(GlassViewEventHandler.java:449)
    at com.sun.glass.ui.View.handleMouseEvent(View.java:551)
    at com.sun.glass.ui.View.notifyMouse(View.java:937)
    at com.sun.glass.ui.win.WinApplication._runLoop(Native Method)
    at com.sun.glass.ui.win.WinApplication.lambda$runLoop$3(WinApplication.java:184)
    at java.base/java.lang.Thread.run(Unknown Source)
carlocastoldi commented 1 year ago

I investigated a little further. I think this has due to QuPath v0.4+ having thoroughly revised the PathClass.

The incriminating lines are the following, to my understanding:https://github.com/BIOP/qupath-extension-abba/blob/d9384ba7e198a454cb8e9da4661988eac7ee285c/src/main/java/qupath/ext/biop/abba/AtlasTools.java#L248 https://github.com/BIOP/qupath-extension-abba/blob/d9384ba7e198a454cb8e9da4661988eac7ee285c/src/main/java/qupath/ext/biop/abba/AtlasTools.java#L260

apparently the objects returned from:

have two different hashes (.hashCode()). I could not reproduce the issue with the following code:

import qupath.ext.biop.abba.*
import qupath.ext.biop.abba.struct.*
import qupath.lib.scripting.*
import qupath.lib.objects.classes.PathClass
import qupath.lib.roi.interfaces.*
import qupath.lib.roi.*

import java.nio.file.Paths;
import java.util.stream.Collectors;

ROI roi = new RectangleROI()
PathObject object = PathObjects.createAnnotationObject(roi)
object.setName("OBJECT NAME");
PathClass parent = QP.getPathClass("Right");
println("parent: "+parent.toString()+" --- hash: "+parent.hashCode())
PathClass pc = QP.getDerivedPathClass(parent, "PATH CLASS"); // "Right\n"+
println("RightPC: '"+pc.toString()+"' --- RightPC hash: "+pc.hashCode()+" --- Parent PC (getClassPath()): '"+parent.toString()+"' --- Parent (getClassPath()) hash: "+parent.hashCode()+" --- parent.isDerivedClass: "+parent.isDerivedClass()+" --- Parent PC (getParentClass()): '"+pc.getParentClass().toString()+"' --- Parent (getParentClass()) hash: "+pc.getParentClass().hashCode()+" --- pc.getParentClass().isDerivedClass: "+pc.getParentClass().isDerivedClass());
object.setPathClass(pc);
object.setLocked(true);

for (var pc_str : PathClass.existingClasses.keySet()) {
    pc = PathClass.existingClasses.get(pc_str)
    if (pc.getParentClass() != null)
        println "${pc.hashCode()} --- key: ${pc_str}, parent: ${pc.getParentClass().hashCode()}, color: ${pc.getColor()}"
    else
        println "${pc.hashCode()} --- key: ${pc_str}, parent: null, color: ${pc.getColor()}"
}
println PathClass.existingClasses.get("Right").hashCode()
println "'Right' PathClass: ${QP.getPathClass("Right").hashCode()}"
println "'${object}' PathClass: ${object.getPathClass().hashCode()}"
println "'${object}' BaseClass (${object.getPathClass().getBaseClass().getName()}): ${object.getPathClass().getBaseClass().hashCode()}"
println object.getPathClass().toString()

output:

INFO: parent: Right --- hash: 9462928
INFO: RightPC: 'Right: PATH CLASS' --- RightPC hash: 1804365739 --- Parent PC (getClassPath()): 'Right' --- Parent (getClassPath()) hash: 9462928 --- parent.isDerivedClass: false --- Parent PC (getParentClass()): 'Right' --- Parent (getParentClass()) hash: 9462928 --- pc.getParentClass().isDerivedClass: false
...
INFO: 9462928 --- key: Right, parent: null, color: -14417340
INFO: 1804365739 --- key: Right\nPATH CLASS, parent: 9462928, color: -12163337
...
INFO: 9462928
INFO: 'Right' PathClass: 9462928
INFO: 'OBJECT NAME (Right: PATH CLASS)' PathClass: 1804365739
INFO: 'OBJECT NAME (Right: PATH CLASS)' BaseClass (Right): 9462928
INFO: Right: PATH CLASS

On the other hand, running AtlasTools.getFlattenedWarpedAtlasRegions() with similar prints gets the following output:

INFO: parent: Right --- hash: 322880273
INFO: GETTING ANNOTATIONS...
INFO: Loading 69 Atlas Regions for 87S_1.czi - Scene #05
INFO: leftROI: Geometry (5691, -2951, 12069, 14278) --- finalTransform: null
INFO: rightROI: Geometry (-4898, -2966, 20880, 16052) --- finalTransform: null
...
INFO: RightPC: 'Right: MOp1' --- RightPC hash: 1107190967 --- Parent PC (getClassPath()): 'Right' --- Parent (getClassPath()) hash: 322880273 --- parent.isDerivedClass: false --- Parent PC (getParentClass()): 'Right' --- Parent (getParentClass()) hash: 463216582 --- pc.getParentClass().isDerivedClass: false
...
INFO: DONE!
INFO: 1107190967 --- key: Right\nMOp1, parent: 463216582, color: -13237402
INFO: 322880273 --- key: Right, parent: null, color: -14417340
INFO: 'Right' PathClass: 322880273
INFO: 'Right' singleton PathClass: 322880273
INFO: 'MOp1 (Right: MOp1)' PathClass: 1107190967
INFO: 'MOp1 (Right: MOp1)' BaseClass (Right): 463216582
INFO: Right: MOp1

as you can see, the two hashes of the two PathClass retrieved in two different ways (from a "child" PathClass and from a String name) are different.

Looping over PathClass.existingClasses (the dictionary used to track the existing classes without duplicates) shows that a PathClass child (i.e., a region), references as a parent a PathClass object (i.e., a "Left"/"Right" hemisphere) that is not in the PathClass.existingClasses HashMap! I don't understand how this is possible. I even tried to briefly look & log QuPath 0.4+, but I was unsuccessful.

I also tried to use PathClass.getSingleton() whenever QP.getPathClass() is called, but it didn't solve the problem.

NicoKiaru commented 1 year ago

Thanks a lot for this investigation. And sorry but I do not know when I will have time to (attempt to) solve this issue. Is this bug super annoying ?

carlocastoldi commented 1 year ago

I think i fixed the problem I previously reported. It was a QuPath 0.4.0+ problem.

I opened a PR: https://github.com/qupath/qupath/pull/1286

carlocastoldi commented 1 year ago

May ask you if you, @NicoKiaru, could please help me answer Pete's questions over in the QuPath's PR? The problem appears to be with how QuPath 0.4.0+ deserialises PathClasses using "legacy"/earlier JSON syntax.

Pete, QuPath's maintainer, said he's keen to understand when the bug strikes. Do you think you can help us provide him a minimal example that shows QuPath 0.4.0+ serializing using the old syntax? Is this something ABBA does when exporting/importing a project?

Do you think this happens only when importing a QuPath <0.4.0 project into QuPath >=0.4.0?

NicoKiaru commented 1 year ago

Do you think this happens only when importing a QuPath <0.4.0 project into QuPath >=0.4.0?

Possible... This would take time to test.

petebankhead commented 1 year ago

@NicoKiaru The issue we see here related to deserializing a derived PathClass from json. I had a look and I couldn't see where in the ABBA extension that would be called though, so maybe it's a different issue - or is there somewhere that ABBA works with json-ed versions of QuPath objects or classes?

If not, can you give examples of what

String name = node.data().get(ontology.getNamingProperty());

would be? If there is a : involved then that might also confused QuPath.

NicoKiaru commented 1 year ago

I'm confused, but you're probably right, I do not see where ABBA would deserialize a json PathClass.

I really need to have a reliable way to reproduce this issue.

@carlocastoldi by any chance do you have a reproducible way, within ABBA, to trigger the error of this issue ? I think I could understand what goes wrong while debugging. (No need to give images, I think I can replace URI by dummy URIs)

petebankhead commented 1 year ago

(I should clarify that @carlocastoldi's fix isn't specific to the json, but will require QuPath v0.5.0... I haven't figured out a way to trigger the problem without using json though, which is why I'm keen to know 'how broken' it is and if anything else could be affected...)

carlocastoldi commented 1 year ago

by any chance do you have a reproducible way, within ABBA, to trigger the error of this issue ? I think I could understand what goes wrong while debugging. (No need to give images, I think I can replace URI by dummy URIs)

TestProject.0.3.2.zip

This project, if opened without my fix, shows inconsistency when running the following code:

pc1 = QP.getPathClass("Right")
pc2 = QP.getDerivedPathClass(pc1, "root").getParentClass()
println pc1 === pc2
println pc1.hashCode()
println pc2.hashCode()

output:

INFO: false
INFO: 940280171
INFO: 1345225187

The annotations of the above project were created with QuPath 0.3.2. I am not sure which version of the extensions were used, but I should be able to retrieve them, if needed. In order to open the project no extension should be needed, afaik

carlocastoldi commented 1 year ago

can this issue be closed, since it was fixed upstream?

NicoKiaru commented 1 year ago

Let's keep it opened until it becomes fixed for real. I think this issue could be related:

https://forum.image.sc/t/regions-disappear-during-import-splitting-left-right-in-qupath/85298/4

carlocastoldi commented 1 year ago

I feel like the symptoms are compatible with the bug i found. If he's able to, I'd ask him to compile and try the version from the main branch of QuPath

NicoKiaru commented 1 year ago

If he's able to, I'd ask him to compile and try the version from the main branch of QuPath

That's quite involved I fear... And I can't ask this for all users.

I feel like I should have gone for naming the annotation instead of setting their pathclass.

imagesc-bot commented 1 year ago

This issue has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/regions-disappear-during-import-splitting-left-right-in-qupath/85298/12

petebankhead commented 1 year ago

@NicoKiaru where are PathClass objects being deserialized here, either from JSON or Java serialization?

Based upon the nature of the bug, I imagine it might be possible to work around it by 'repairing' broken PathClass objects or intercepting the deserialization, but from a quick look through of the code I couldn't tell where either of those would be required.

NicoKiaru commented 1 year ago

When the user executes the command

image

the PathClass are created 'from scratch', as @carlocastoldi mentioned in the lines:

https://github.com/BIOP/qupath-extension-abba/blob/d9384ba7e198a454cb8e9da4661988eac7ee285c/src/main/java/qupath/ext/biop/abba/AtlasTools.java#L248

and

https://github.com/BIOP/qupath-extension-abba/blob/d9384ba7e198a454cb8e9da4661988eac7ee285c/src/main/java/qupath/ext/biop/abba/AtlasTools.java#L260

image

Then the ROIs are serialized when the user saves its currently modified entry, and deserialized when the user reopens its image entry.


A few extra notes:

Sorry if what I explain is not clear or not what you asked. It's very unclear to me what's going wrong here, and I can't find a reproducible way to trigger a somewhat related bug.

Maybe I should 'pre'create all the PathClass in QuPath before doing any ROI import ? Maybe I should just name the annotations according to the atlas regions instead of setting a PathClass with this name <- this would probably solve everything. I would just need to fix a bunch of scripts if this logic is modified ( for instance, https://forum.image.sc/t/qupath-script-to-restrict-cell-detection-to-several-sub-regions-of-the-brain/71707/3)

petebankhead commented 1 year ago

Quick update: this seems one of the most subtle/annoying/bad bugs I've seen in QuPath (thanks again to @carlocastoldi for finding it!) - although clearly bad consequences seem mercifully rare.

To summarize: QuPath classes should always be singletons, and they always are when created directly within QuPath. But when deserializing a derived class, the parents aren't forced to be singletons. This bug was first introduced in v0.4.0 and remains in v0.4.3.

It potentially affects a lot. If I do a 'standard' analysis for tumor positive cells - i.e. cells classified as Tumor: Positive or Tumor: Negative - then reopen the image data in a new QuPath session I can see that none of the cells have a base classification that matches the singleton class for Tumor... even if, in fact, all of them should. In other words, this always prints zero:

def tumor = getCellObjects().findAll {
    it.getPathClass()?.getParentClass() === getPathClass('Tumor')
}
println(tumor.size())

However, Groovy helps mitigate things (and avoid many more scripts breaking) because its default == equality uses PathClass.compareTo and this actually works. So I get the 'right' result with:

```groovy
def tumor = getCellObjects().findAll {
    it.getPathClass()?.getParentClass() == getPathClass('Tumor')
}
println(tumor.size())

I worry that there are more subtle consequences of the bug in QuPath, so the fix is urgent. But my impression/hope is that they can't be that common, or they'd have been reported by now.

Anyhow, back to this issue: the problem in the original post stems from identifying the left and right annotations with the filters at https://github.com/BIOP/qupath-extension-abba/blob/d9384ba7e198a454cb8e9da4661988eac7ee285c/src/main/java/qupath/ext/biop/abba/AtlasTools.java#L91C35-L91C91

Rather than

.filter(po -> po.getPathClass().isDerivedFrom(QP.getPathClass("Left")))

a more robust comparison could be

.filter(po -> "Left".equals(po.getPathClass().getBaseClass().getName()))

and the corresponding updated filter for "Right".

Basically, any time you can extract strings from PathClass objects and then compare with them, the bug should be avoided.

Use of PathClass.compareTo also works, but PathClass.equals() and PathClass == PathClass don't work reliably in QuPath v0.4.x (although == is ok in Groovy, since it's really a compareTo... and === is the broken Groovy operator)

petebankhead commented 1 year ago

I've now written up the issue thoroughly at https://github.com/qupath/qupath/issues/1306

I've also created a PR here: https://github.com/qupath/qupath/pull/1307

Could anyone check if that resolves things? If so, we could release it as v0.4.4 with just that one fix.

GuillaumeLeGoc commented 1 year ago

Hi, I built QuPath v0.4.4 in Windows 10 and I was not able to reproduce this related issue.

Thanks for looking into it and coming up with a fix and thanks to @carlocastoldi for pinpointing it :)

petebankhead commented 1 year ago

Excellent, so I think all the evidence points to v0.4.4 fixing it. If no one spots any problems in the next hour or two, we plan to make the release today :)

carlocastoldi commented 1 year ago

Now that QuPath 0.4.4 was released, this issue should be be finally closable, right? ^^

NicoKiaru commented 1 year ago

@carlocastoldi Indeed! I've put warnings in all the repos and put a post on the forum. I'll put a small startup message on Fiji, and I think we should be good.

Thanks again to all of you!

carlocastoldi commented 1 year ago

Thank you everyone for the rapidity in tackling the issue, once the causes were found!

imagesc-bot commented 1 year ago

This issue has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/regions-disappear-during-import-splitting-left-right-in-qupath/85298/17

NicoKiaru commented 1 year ago

I've also added a warning message on Fiji's side:

https://github.com/BIOP/ijp-imagetoatlas/commit/61e5d31b251bcbe728ce3c63e0519451885d7a67