Manders real noisy test

[chalkie666]

I fear I discovered a maths bug in Manders colocalisation coefficients, using thresholds above or below (none thresholds was correct).

@tomka Please check the Manders paper (it linked ot on the Coloc_2 page) and verify in code review.

I spotted the logic was wrong in the boolean code that selects if the cursor is looking at a pixel which is above the threshold in the OTHER channel. We were looking in the SAME channel, which was wrong.

Our unit test cases for the artificial Manders Test images did not catch this... I think because the 2 channel test images of the Manders Gaussian blobs/spots have the same thresholds and range... they are identical really, so it doesn't matter if the wrong channel threshold is checked.... The original test still passed with the bug fix in this branch's code.

I implemented a unit test method mandersRealNoisyImagesTest() using a newly created mask image ( coloxsample1b-mask.tif) which I added to the repository) for the positive correlation "colocsample1b" sample image data - i verified that this gives pretty different Manders coefficient (thresholded) results with and without this bug fix. See attached pdf outputs.

Note the PDFs were not created with the same mask image as in this PR... i had to recreate it, so the numbers might not be reproducible. But the unit test does fail with the buggy version of the Manders algorithm code. Hopefully it will catch all sorts of other problems too, as the accuracy of the unit test is set very high.

I noticed this bug upon re-reading the Manders paper, checking the old plugin's code - Colocalization Threshold, and that in BioImageXD.net, hearing about differences between Manders results from Coloc_2 and other software (they all make different numbers anyway....) and while working on adding more split coloc coefficient to the MandersColocaization Class (as requested by Ann Wheeler in issue #17....

There is also some renaming of variables and a class, new variables, and a bunch more detailed comments, in order to make things more understandable in light of these bugs and the next part of the work to add more split coefficients.

I hope i got all the whitespace and the coding style is ok... if not, feel free to fix... I installed the eclipse template things for code style... but not sure how to use them... maybe they just work...

cheers

Dan

coloc_colocsample1b-green.tif_colocsample1b-redFixedManders.pdf coloc_colocsample1b-green.tif_colocsample1b-red.tif_mask_1_OnlyDiffWongMandersAbove.pdf

[chalkie666]

@etarena if you are interested about this colocalization stuff, here is a good chance to learn about one of the commonly used measurements: split Manders' coefficients. Have a look at the logic change in commit 7a42ae5 https://github.com/chalkie666/Colocalisation_Analysis/commit/7a42ae5e02ed7666dad3dd4affa195106a89ac63 and see if it matches the description of the maths in the original paper http://fiji.sc/_images/2/24/Manders.pdf If at least one person agrees with the change in commit 7a42ae5 then i will merge it, as its an output bug - wrong maths. @tomka maybe you cant get onto this right now, but thats ok so long as someone/anyone else verifies i made the right change in commit 7a42ae5

[etadobson]

@chalkie666

thanks dan. i can take a look at this first thing tomorrow... i'll be on it.

[ctrueden]

So, what's the consensus thus far? Merge? Or not yet ready for merge?

[chalkie666]

@ctrueden Eta checked the maths in the original paper, page 2 equations 6 and 7, as linked to above, and here: http://fiji.sc/_images/2/24/Manders.pdf and agrees with the change. So that's two pairs of eyes. If one more pair of eyes attached to a brain agrees, then merge.

@tomka is clearly busy doing real work, so we should not pester him to respond... rather find an independent fresh brain to double check it.

if merge, then we should also cut a release, as this is a fix that affects scientific results and interpretation of them. Most of the time probably not much difference.... but for correctness it should be clear to users.

[ctrueden]

@chalkie666 My brain is hardly fresh. It smells like pedantry and stale PRs in here.

I made some nitpicky comments. Nothing on the math—I couldn't see the math changes amongst all the renames and stuff in the same commits.

@etarena I saw you mention line numbers. For future reference, note that GitHub lets you comment on individual line numbers, so you shouldn't ever need to write such numbers explicitly—just comment at the appropriate places as needed by clicking on the line number links on the left of the diffs.

At minimum, before merging I would like to see the commit adding the png removed, and the one adding the tif amended to simply say that it adds the TIFF image. And only if that TIFF image is actively used by an automated test—otherwise it just bloats the repository size.

I would ideally like to see a working unit test exercising the correct behavior, rather than WIP commits stating that the test still does not work. It sounds like @dietzc identified the main bug in your test—any progress on a working version of the test?

[chalkie666]

@ctrueden later commits fix the unit test which now does what it should and detects wrongness and changes to the maths.

[ctrueden]

@chalkie666 I'll try to get this merged right now, cleaning up the history as needed.

[ctrueden]

OK, I cleaned up the history a bit, purging versions of the test image besides the final one. I also made some other minor edits to the history.

The result is as follows:

$ check-branch.sh
0000 9dc4bf766599e3bfaae83d36d1a729e4374894b4 SUCCESS 29
0001 6e5c528cd7b0afc9f79e82cbd11f4e7ff329252d SUCCESS 29
0002 265f6740e35af8104e969b55bb9c61265999508f SUCCESS 30
0003 9c4ba7d4d5c6bcff55755f85deff0267659b903e SUCCESS 29
0004 9ab837c58ede436804628eb2a45a3979bf025334 FAILURE 7
0005 af35cf6f505f37981ab636d2336def4a6105bcb9 FAILURE 7
0006 217134649dd21e39d867ade1c8181bcf1199e722 FAILURE 23
0007 ed58a1678f372d955d575a738319044d1fc8a592 FAILURE 23
0008 c4efa7d944109a1f4766d3de38b5b06be06b9210 SUCCESS 30
0009 349b86ca643a826031a6dd30b76e87144b51a0a2 SUCCESS 30
0010 06b4165f73fdb40115f58a1e0615296cee4b292b SUCCESS 29
0011 b37c6f0a0bbe6b0929efad6d2f3c9c2ab8ebc7a9 SUCCESS 30

For reasons of time, I decided not to clean up the four failing commits, and just merge it. In the future, each commit should ideally compile with passing unit tests. Otherwise, debugging with git bisect stops working effectively.

[chalkie666]

@ctrueden actually, right now , I'm doing rebase now of my local branch and fixing/editing commit messages, and squashing commits, and all that jazz....and rewriting my history as suggested in the articles you linked to... and fixiing the stuff you popinted out in above comments.... so it is possible to undo that merge you just did of this branch, wait till im done rebasing and fixing and tidying and have force pushed to this github branch, to update this PR to more how you would like it.... and then if its ok, we can agree to pull it into fiji with a merge like you just did.

[ctrueden]

@chalkie666 OK, since you want to keep working on it, you can force push to your branch again after getting it into the shape you desire. We can then revert my earlier merge, and merge your version. Just let me know when you are ready.

[chalkie666]

@ctrueden ok will do, sorry for not letting you know i was doing it before you took the bull by the horns.Your good intent and support certainly receive my thanks!

[ctrueden]

@chalkie666 I'm confused—did you force push this yet? Is it ready for review/merge?

Unfortunately, I am insanely busy preparing the for the ImageJ conference right now. That's why I just went ahead and did the cleanup and merge myself. I really appreciate your enthusiasm and drive to learn, and I want to help you—the timing is just really tough right now. I wish @tomka could review this—he really is better suited to judge the colocalization math, and he knows Git well too.

[chalkie666]

nope not yet.... nearly ready...need to redo the important commit that fixes the bug so the 4 changed lines are clear in the history and not mixed up with a munch of other stuff... I will let you know when I force push to my remote that is the PR branch Tom is on holiday... so yes its just double bad timing.

cheers d

On 28 August 2015 at 18:48, Curtis Rueden notifications@github.com wrote:

@chalkie666 https://github.com/chalkie666 I'm confused—did you force push this yet? Is it ready for review/merge?

Unfortunately, I am insanely busy preparing the for the ImageJ conference right now. That's why I just went ahead and did the cleanup and merge myself. I really appreciate your enthusiasm and drive to learn, and I want to help you—the timing is just really tough right now. I wish @tomka https://github.com/tomka could review this—he really is better suited to judge the colocalization math, and he knows Git well too.

— Reply to this email directly or view it on GitHub https://github.com/fiji/Colocalisation_Analysis/pull/28#issuecomment-135829905 .

[ctrueden]

Tom is on holiday... so yes its just double bad timing.

That's what we get for hosting a conference on Labor Day weekend. Everyone and his dog is on vacation this week!

[chalkie666]

@ctrueden I've finished rebasing. I reworded, squashed, edited, split, and generally rewrote the history to make it clean, and only have commits that compile and aren't broken, as you asked for. I'm not saying the history now looks perfect, but its much clearer.

If this is ok, then we should cut a release, as the bug fix for Manders' split coefficients does change scientific results. I made the commit for that fix e4a3a6e contain just the 4 lines changed, so the important bit isn't lost in the noise. I gave it an explanatory commit message.

Now I see it wont auto merge... i guess that's due to the rebasing.... should i try to fix that or is it easier for you to do it in a few magic git commands? You mentioned that you could revert the latest merge you did then apply this ...?

[chalkie666]

@ctrueden thanks!!!

[ctrueden]

@chalkie666 Thanks for taking the time to clean things up. The conflicts were quite trivial (only with the README, and some javadoc in Coloc_2.java)—easily resolved from the CLI.

[chalkie666]

@ctrueden It was fun to remind myself how to rebase -i after a long time away from the git cli and only ever having watched Tom do it over his shoulder, while i tried to digest the coloc algorithms in the papers ....☺

On 31 August 2015 at 21:52, Curtis Rueden notifications@github.com wrote:

@chalkie666 https://github.com/chalkie666 Thanks for taking the time to clean things up. The conflicts were quite trivial (only with the README, and some javadoc in Coloc_2.java)—easily resolved from the CLI.

— Reply to this email directly or view it on GitHub https://github.com/fiji/Colocalisation_Analysis/pull/28#issuecomment-136481500 .