igvteam / igv

Integrative Genomics Viewer. Fast, efficient, scalable visualization tool for genomics data and annotations
https://igv.org
MIT License
646 stars 387 forks source link

Improvements to Supplemental Read navigation and display #1412

Closed lbergelson closed 1 year ago

lbergelson commented 1 year ago

Major rework of the previous alignment dialog popup to make it interactive and clearer. Use it to navigate between reads in a supplemetnary alignment group. Click to move to the selected read. Shift + Click to add it as a split screen.

When jumping to a selected read/mate the selected reads will be now be sorted to the top. @jrobinso There's currently a nondeterministic bug here, sometimes the sorting crashes with an NPE because the interval isn't loaded. I'm misunderstanding something about the loading code. It would be easy to fix it so it doesn't blow up by checking the interval for null, but then we'd just have the problem that it sometimes isn't putting the selected reads at the top mysteriously. I left it to blow up so that it's obvious when it happens so we can notice / fix it. I'm a bit puzzled by it

java.lang.NullPointerException: Cannot invoke "org.broad.igv.sam.AlignmentInterval.sortRows(org.broad.igv.sam.SortOption, double, String, boolean, java.util.Set)" because "interval" is null
        at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315)
        at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:320)
        at java.base/java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1807)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.lang.NullPointerException: Cannot invoke "org.broad.igv.sam.AlignmentInterval.sortRows(org.broad.igv.sam.SortOption, double, String, boolean, java.util.Set)" because "interval" is null
        at org.broad.igv.sam.AlignmentTrack.lambda$sortRows$2(AlignmentTrack.java:767)
        at org.broad.igv.sam.AlignmentTrack.lambda$receiveEvent$1(AlignmentTrack.java:402)
        at java.base/java.util.HashMap.computeIfPresent(HashMap.java:1261)
        at org.broad.igv.sam.AlignmentTrack.receiveEvent(AlignmentTrack.java:401)
        at org.broad.igv.event.IGVEventBus.post(IGVEventBus.java:83)
        at org.broad.igv.sam.AlignmentDataManager.load(AlignmentDataManager.java:361)
        at org.broad.igv.sam.CoverageTrack.load(CoverageTrack.java:185)
        at org.broad.igv.ui.IGV.lambda$repaint$16(IGV.java:2338)
        at java.base/java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1804)
        ... 3 more

Renamed several classes and moved them to the ui/supdiagram package Refactoring some methods around Frame creation, this could still be improved.

jrobinso commented 1 year ago

@lbergelson when does this lambda function (sort) ever get removed from the actionToPerformOnFrameLoad cache? The purpose seems to be to defer sorting for this frame, but as far as I can see this never gets removed. Won't it get invoked repeatedly, rather than once (which seems to be the intent).

This doesn't seem to be added in this PR, I just noticed it looking into the bug you posted above.

actionToPerformOnFrameLoad.put(frame, sort);
lbergelson commented 1 year ago

@jrobinso It should be removed when it's executed. If you look at where it's called, it's in a "computeIfPresent" block and after executing it returns null which removes it. I could replace it which a remove() and != null check if you think that's more readable.