Fix critical bugs in Manders calculations pixel threshold boolean selection

[chalkie666]

I fear I discovered a maths bug in Manders colocalisation coefficients, using thresholds above or below (none thresholds was correct). 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.

Out unit test cases did not catch this... i think bacause the 2 channel test images of the spots have the same thresholds and range, so it doesnt matter if the wrong channel threshold is checked.... The tests still pass woh this branch's code. I suggest a unit test using the mask image (see below) for the colocsample1b sample image data - i verified that this gives pretty different Manders coefficient (thresholded) results for this bug fix. See attached pdf outputs.

I noticed this upon re-reading the Manders paper, checking the old plugin's code - Colocalization Threshold, 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....

Also here is a mask image to go with the colocsample1b test images to use as a ROI/mask in the analysis and a unit test.

There is also some renaming of variables and a class, 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-red.tif_mask_1_OnlyDiffWongMandersAbove.pdf coloc_colocsample1b-green.tif_colocsample1b-redFixedManders.pdf

[tomka]

Thanks Dan, I try to look into this in the next days.

[chalkie666]

thanks Tom! and thanks for your latest pull request, i merged it. Are there still some more of my changes to add.., you hinted that was the case...

Thanks again for taking the time to review my code and administer this. Its really appreciated by me and the users!

(we need to write a coloc2 paper when we are done with the next round of improvments, so you can get cited!!!)

cheers

d

On 3 August 2015 at 17:30, Tom Kazimiers notifications@github.com wrote:

Thanks Dan, I try to look into this in the next days.

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

[chalkie666]

@tomka i didnt realise the pull request would also include further changes i made.... so this branch now also includes unfinished and very borken stuff... so I only meant the pull request to merge the two commits from Aug 3rd. 134a66d and 7a42ae5 so now i know i need to branch again after a pull request... sorry.

[chalkie666]

@tomka I figured out how to revert my last two commits in this branch, so now it would be ok to merge with master if you thinks the Maders' coeff fox and the other trivial commits are ok . I made myself a new branch for the Manders Tests to catch the bug this pull request fixes. Sorry for being so hopeless. I hope i didn't create extra work for you. cheers, D

[tomka]

Yeah, you basically make a single branch a pull request. Everytime you add to the branch, the PR will be updated, too. Therefore, it is useful to use topic branches and don't add unrelated things to add. For this PR your could have created a branch called e.g. fix-manders-thresholding-bug which would contain the first two commits in this branch and continue developing in another branch.

[tomka]

Also, instead of reverting to not clutter history, you could just reset this branch to commit to e.g. 7a42ae5 and force push to this branch (which you should basically never do with published branches that others potentially work with):

git checkout fraction_colocs
git reset --hard 7a42ae5
git push origin +fraction_colocs

The + indicates a force push, you would override history. You would need to make sure, though, that the commits you "loose" this way are part of another branch. Otherwise, they will be garbage collected by git at some point. However, a saver approach would be to create a new branch, set this to the commit you want, close this pull request and create a new one:

git branch fix-manders-thresholding-bug 7a42ae5
git push origin fix-manders-thresholding-bug 7a42ae5

And then create a PR from this branch.

[chalkie666]

I'm such a git no hoper. It makes my head spin. But the more I do it the more tricks I will learn. I was trying to rely on the windows Github client. But it can't Do all that fancy reset, rebase, reset stuff. At least it can revert and branch.

I will start using the windows git shell more I guess.

Thanks for all the git tips. Very very helpful.!!!

Cheers. D On 5 Aug 2015 14:26, "Tom Kazimiers" notifications@github.com wrote:

Also, instead of reverting to not clutter history, you could just reset this branch to commit to e.g. 7a42ae5 https://github.com/fiji/Colocalisation_Analysis/commit/7a42ae5e02ed7666dad3dd4affa195106a89ac63 and force push to this branch (which you should basically never do with published branches that others potentially work with):

git checkout fraction_colocs git reset --hard 7a42ae5 git push origin +fraction_colocs

The + indicates a force push, you would override history. You would need to make sure, though, that the commits you "loose" this way are part of another branch. Otherwise, they will be garbage collected by git at some point. However, a saver approach would be to create a new branch, set this to the commit you want, close this pull request and create a new one:

git branch fix-manders-thresholding-bug 7a42ae5 git push origin fix-manders-thresholding-bug 7a42ae5

And then create a PR from this branch.

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

[chalkie666]

@tomka ok, i went with the first option, and reset two commits back, and pushed it to github. So hopefully thats ok to merge without irrelevant commits in the history.

Now i understand the branching and new branch for a pull request workflow... sorry for being slow to get it.

[tomka]

Great, thanks for fixing this and looking more into branching and pull request workflows! And no worries, git can be a complex tool sometimes.

[chalkie666]

I will close this pull request, and open another that contains the Manders algorithm fix in thsi PR, plus a unit test to catch that bug and hopefully others.

[chalkie666]

These commits are also in PR #20 so can close this one Not sure how I ended up with two Prs with the same commits....anyway, my bad ...

[ctrueden]

You mean "These commits are also in PR #28", right?

[chalkie666]

yes, sorry, i was confused, i did mean PR #28