Add SACA framework Op and SACA use case #156

elevans · 2024-04-29T20:28:18Z

This PR adds the SACA framework (3 Ops: z-score heat map, p-value heatmap, sig mask) and the use case that goes with it.

This commit adds Shulei and Ellen's SACA code from imagej/imagej-ops coloc-multithread-saca branch from commit b12ce2904945f7cf14db105e86f917d113279b63. This is right before Ellen commited a WIP first attempt to multithread SACA. While it works there are notes indicating that the output is still slow and does not conform to Shulei's output from the original R package. Given this, I felt it best to start the SciJava Ops port from the last known working state and then make progress towards resolving the Op performance and output results in the SciJava Ops framework.

This commit converts the imagej op copied from the saca branch to a SciJava op. This Op works, however it is not multithreaded and it produces NaN results at the edge of the heatmap.

Shulei's original SACA code also sets NaN's to 0.

To multithread the singleiteration function, we need to use the chunked LoopBuilder. This results in about a ~40% reducing in processing time.

The SACA framework has three components: - Z score heatmap - SigPixel matrix - pValue matrix The Z score heatmap is the result of weighted kendall tau operation. The SigPixel (significant pixel) and pValue results are derived from the heatmap Z score. Thus to use the SigPixel and pValue matrix outputs the user needs to first create the Z score heatmap.

The goal here is to let Adaptation make this op a Function.

This commit is a rebase of the saca branch with main. For some reason the simplify/RAIFocusersTest.java file was kept and not removed during the rebase. This commit removes it and updates the OpRegressionTest count.

The loop builder introduces a non-reproducible randomness to the SACA algorithm. Unfortnately, we need stop using the multithreaded version of the loop builder to resolve this.

This commit adds a test for the SACA framework. It tests all three Ops in the "coloc.saca" namespace. A slice is taken of both input test data and processed using SACA. 10 positions are sampled on all three SACA outputs.

gselzer

@elevans by and large this looks great! Thank you for porting this - just a couple small changes, none of which I'd expect to take any real work.

The one strange thing here is that there are edits to RAIFocusers.java and related templates which are probably unnecessary. Can we see if this PR still works without those changes? Same goes for the edits to SciJava Ops Engine changes - I think I put comments on each

scijava-ops-image/templates/main/java/org/scijava/ops/image/simplify/RAIFocusers.vm

scijava-ops-image/templates/test/java/org/scijava/ops/image/simplify/RAIFocusersTest.vm

scijava-ops-image/templates/main/java/org/scijava/ops/image/simplify/RAIFocusers.list

scijava-ops-image/templates/test/java/org/scijava/ops/image/simplify/RAIFocusersTest.list

scijava-ops-engine/src/main/java/org/scijava/ops/engine/impl/DefaultOpEnvironment.java

scijava-ops-image/src/main/java/org/scijava/ops/image/coloc/saca/SACAHeatmapPValue.java

scijava-ops-image/src/main/java/org/scijava/ops/image/coloc/saca/WtKendallTau.java

scijava-ops-image/src/main/java/org/scijava/ops/image/simplify/RAIFocusers.java

scijava-ops-image/src/test/java/org/scijava/ops/image/coloc/saca/SACATest.java

elevans · 2024-04-29T22:28:30Z

@gselzer Absolutely! So the RAIFocusers commit came from a rebase that had merge conflicts. I probably messed this up. So if this works without these changes, Great!!

hinerm · 2024-04-30T21:21:51Z

I don't see anything that needs revision beyond what Gabe said. Thanks @elevans!

elevans · 2024-04-30T21:27:52Z

Thanks! Almost done!

I accidently left these RAIFocusers in from a rebase.

This commit removes some code changes that I think I applied from a rebase from one experimental branch to the SACA one. I checked the log history in main and couldn't find the code changes there. Regardless, they are not needed. This commit should now align with the current ops engine in main.

elevans · 2024-05-01T15:45:33Z

@gselzer Thanks for the awesome review. I think I've replied to everything and addressed all comments. Its ready for another look!

gselzer

@elevans thank you so much for addressing my prior review! More Ops = a better framework!

...unfortunately, I'd suggest a couple more things 😅

docs/ops/doc/examples/saca.rst

scijava-ops-image/src/main/java/org/scijava/ops/image/stats/DefaultPNorm.java

scijava-ops-image/src/test/java/org/scijava/ops/image/coloc/saca/SACATest.java

scijava-ops-image/src/test/java/org/scijava/ops/image/stats/DefaultPNormTest.java

This commit converts the PNorm.java helper class into an Op in the `stats` namespace. This commit also adds a test for this new Op.

This commit converts the QNorm.java helper class for SACA into an Op. This commit also includes a test for this new Op.

This commit refactors the coloc.saca namespace tests

This commit refactors the SACA test to use a setup helper method create the resources needed for the tests just once.

elevans · 2024-05-02T16:13:16Z

@gselzer This PR is ready for its second round of review. Thanks for all the help!

gselzer and others added 18 commits April 29, 2024 15:26

Add RAI focusers

aff6770

Convert SACA ImageJ Op to SciJava Op

b01bb1a

This commit converts the imagej op copied from the saca branch to a SciJava op. This Op works, however it is not multithreaded and it produces NaN results at the edge of the heatmap.

Add check for NaN results and set to 0

f808ad1

Shulei's original SACA code also sets NaN's to 0.

Use multithreaded Loopbuilder for SACA

449d2bc

Use chunking with LoopBuilder in singleiteration

4b1124f

To multithread the singleiteration function, we need to use the chunked LoopBuilder. This results in about a ~40% reducing in processing time.

Add QNorm class from imagej-ops saca branch

4f3f5ac

Convert SACA op from a Function to Computer

d5486c4

The goal here is to let Adaptation make this op a Function.

Add SACA significant pixel mask output

c419f28

Add SACA PValue heatmatp op

dbd0628

Clean up imports and improve SACA Op descriptions

658fd2d

Rebase with main -- remove arity() syntax

5fffc46

This commit is a rebase of the saca branch with main. For some reason the simplify/RAIFocusersTest.java file was kept and not removed during the rebase. This commit removes it and updates the OpRegressionTest count.

Use non-multithreaded loop builder

2828449

The loop builder introduces a non-reproducible randomness to the SACA algorithm. Unfortnately, we need stop using the multithreaded version of the loop builder to resolve this.

Limit the SACA heatmap z score op to 2D

c1b33e3

Apply maven formatter, make it happy

bf948cf

Add test for SACA framework

43df122

This commit adds a test for the SACA framework. It tests all three Ops in the "coloc.saca" namespace. A slice is taken of both input test data and processed using SACA. 10 positions are sampled on all three SACA outputs.

Add SACA use case

e3070a5

elevans requested review from ctrueden, hinerm and gselzer April 29, 2024 20:28

gselzer requested changes Apr 29, 2024

View reviewed changes

elevans added 3 commits May 1, 2024 08:15

Remove simply/RAIFocusers

6c9419a

I accidently left these RAIFocusers in from a rebase.

Remove resolved TODOs in kendelltau helper class

6c8e275

elevans force-pushed the scijava-ops-image/saca branch 2 times, most recently from 96713b3 to 63780ff Compare May 1, 2024 15:38

gselzer requested changes May 1, 2024

View reviewed changes

elevans force-pushed the scijava-ops-image/saca branch 2 times, most recently from 15c858b to 9f4a1b4 Compare May 2, 2024 16:06

elevans added 11 commits May 2, 2024 11:10

Convert SACA PNorm helper class into an Op

5d06e5d

This commit converts the PNorm.java helper class into an Op in the `stats` namespace. This commit also adds a test for this new Op.

Convert SACA QNorm helper class into an Op

770d565

This commit converts the QNorm.java helper class for SACA into an Op. This commit also includes a test for this new Op.

Split SACA framework test into two

9b3a565

Update saca use case to use "stats.pnrom"

b9d6666

Fix SACA tests

20226c2

This commit refactors the coloc.saca namespace tests

Enable task progress for coloc.saca.heatmapZScore

997488f

Refactor PNorm Op into a pixel-wise Op

6f647e4

Update SACA example URLs and add coloc values

c38e706

Refactor Abstract coloc test to use static methods

12ca5f8

Refactor SACATest to use setup helper method

7b7d83b

This commit refactors the SACA test to use a setup helper method create the resources needed for the tests just once.

Refactor PNorm test to use ArrayImgs

33cd59e

elevans force-pushed the scijava-ops-image/saca branch from 9f4a1b4 to 33cd59e Compare May 2, 2024 16:10

gselzer approved these changes May 2, 2024

View reviewed changes

gselzer merged commit 7c2fe15 into main May 2, 2024
2 checks passed

gselzer deleted the scijava-ops-image/saca branch May 2, 2024 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SACA framework Op and SACA use case #156

Add SACA framework Op and SACA use case #156

elevans commented Apr 29, 2024

gselzer left a comment

elevans commented Apr 29, 2024

hinerm commented Apr 30, 2024

elevans commented Apr 30, 2024

elevans commented May 1, 2024

gselzer left a comment

elevans commented May 2, 2024

Add SACA framework Op and SACA use case #156

Add SACA framework Op and SACA use case #156

Conversation

elevans commented Apr 29, 2024

gselzer left a comment

Choose a reason for hiding this comment

elevans commented Apr 29, 2024

hinerm commented Apr 30, 2024

elevans commented Apr 30, 2024

elevans commented May 1, 2024

gselzer left a comment

Choose a reason for hiding this comment

elevans commented May 2, 2024