clean up and simplify plugin results outputs before working on new functionality

[chalkie666]

Hi guys, here are a bunch of fairly trivial clean up changes, and a couple of more weighty ones.

I made the three ways results can be expressed to the user show all the same results, no matter which way of seeing the results they choose: I added all results/stats computed and displayed so far into the DataContainer, so all ResultsHandler implementations (SingleWindowDisplay, PFFWriter and IJ.log) just use all the same results values, including a new one called jobName, that uses a unique ID from the imglib2 maskHash int value. results from different masks or ROIs in the same imput images will get different filenames/titles/top line of results.

I changed one of the warnings in the autothreshold algorithm to be more robust, hopefully, and added detail to the warning text.

I outlined the approach for a new algorithm to add (% Coloc by intensity and/or pixels) with some dead code files and a dead code unit test ( implementing it all.... that's my next task)

I wrote a bunch of TODOs in the imagej.net/Coloc_2 wiki page. Should i use the imsgeJ2 issue tracker instead?

I tested my changes manually... its mostly GUI stuff i could not easily make unit test for? Would that be appropriate for these kinds of things?

[ctrueden]

Awesome, @chalkie666!

I wrote a bunch of TODOs in the imagej.net/Coloc_2 wiki page. Should i use the imsgeJ2 issue tracker instead?

I think this issue tracker (https://github.com/fiji/Colocalization_Analysis/issues) is the best place for TODOs, not the wiki.

[ctrueden]

@tomka Could you please review and merge? Much appreciated.

[tomka]

Thanks Dan! I try to have a look at it tonight and hopefully merge it in.

[chalkie666]

hi chaps,

looks like tomka still has another unmerged pull request,... and some of the same files and lines of code are also changes in my pull request... even to my untrained eye it looks like there might be merge conflicts if both pull requests are merged....? Whats the usual way out of this hole? My git foo isnt up to it i fear.

i will add things from the todo list on the coloc2 wikipage to the issue tracker -the things that are concrete problems with a doable solution.... more vague stuff can stay there until its more well defined.

[ctrueden]

Whats the usual way out of this hole?

The simplest way is:

1. Merge the bigger / more disruptive one.
2. Merge the other one, manually, from the CLI.
3. Fix all conflicts and commit the merge.

Another way is to rebase the second branch over the new master after merging the first, fixing conflicts as they occur with each commit. Then force push, then GitHub will be happy again. But this requires a higher level of Git understanding.

[ctrueden]

There are also downsides to rebasing: you create "new" commits that have not actually been tested, unlike your original ones. So they might be broken (behavior-wise, or not even compiling) even though your original branch worked.

Here is one article which compares merge and rebase.

[tomka]

Yeah, the other PR is still not merged, sorry. I try to review both tonight. I am fine with rebasing my changes on yours, I have to go through them again anyway. Let's see what is easier. A merge should otherwise be fine as well, I am okay with resolving this on the CLI.

[chalkie666]

ok, thanks tomka! i will look forward to seeing the merge before i start the next round of hacking to add the %Coloc algorithm.

I noticed in DataContainer there are three implementations of the same method with a lot of the same lines of code. its where i put the code to make the new jobName.... should/can that be made cleaner? I just noticed that i copy pased the same stuff in each implementation - three locations... which feels a bit wrong???

[chalkie666]

Do we bump up the version number when tomka merges both pull requests?
If it's merged then it all goes straight into the updater right?
So I need to update the wiki docs with a release notes ideally I guess?

[ctrueden]

If it's merged then it all goes straight into the updater right?

Nope. Gotta cut a release, then update Fiji to depend on that release.

See the Releases page.

[tomka]

Hi Dan, I looked a bit through your commits and from a functional point of view everything looks fine. But I have some minor points:

1. Don't comment code. If it is not needed, remove it and use the comment message to explain why if it was there before.
2. Like you already mention above, if there is repeated code, it is usually means there is room for improvement. Wrap it in a function or a variable/field.
3. Keep commits to the point and don't mix logically independent changes.
4. Don't shorten lines unless it really improves readability. I think it is a good goal to stay consistent and try to a maximum of 80 characters on a line (which of course doesn't always work).
5. Try to reduce extra/unneeded white-space.

These are more general remarks, but would you mind if I fix some of the coding style issues?

And would you mind leaving the failing unit test out for now until this feature is implemented? I am not sure, but I could imagine Jenkins would otherwise complain (would it @ctrueden ?). Besides that, it is nice that you keep test cases in mind!

If the coding style is more consistent with the rest, the dead/commented code gone and all tests passing I am happy to merge this (and fix merge conflicts due to me having merged the other PR). Let me know with what I can help.

[chalkie666]

Hi Tom,

Thanks for looking at it, and I will follow your advice on those 5 points. You can see I am a bit out of practice!

I commented out code so the reviewer could see what I removed a but easier. But I guess with git you see that very easily anyway, right? So I will be more draconian in future and let git tell us what's removed.

Sure, the empty test was just there because I wanted to see how eclipse auto generated it.... I should have removed it again or not committed it.

I tried hard to kill all the whitespace by looking for it in the windows Github client unused to commit and push. Looks like I need to look harder!

I hope the merge went without major hassles.

Thanks Curtis for the link to the releases info page. Now I understand better what happens. Things have become more sophisticated and robust over the years!!

I vote for a bump to the next minor version since the output results content have changed?

Curtis, When Tom is done merging, will you cut the release and upload to ImageJ2 update site? Or should one of us do it?

I read that coloc_2 is a standard core Fiji component still. That's nice! I wondered if moving it to its own repository meant that it was now no longer a core component. .

[hinerm]

@tomka

And would you mind leaving the failing unit test out for now until this feature is implemented? I am not sure, but I could imagine Jenkins would otherwise complain

Failing unit tests will fail the Jenkins build and prevent deployment of the build artifacts. Test driven development is an excellent practice, but it would be best to put the unit test on its own branch for the new feature.. that can be merged when the feature is ready.

@chalkie666

I tried hard to kill all the whitespace by looking for it in the windows Github client unused to commit and push. Looks like I need to look harder!

We actually provide Eclipse formatting templates. These will keep things nice and consistent with whitespace, tabs, etc. If you add them to your Eclipse you can then select the code you've changed and do Source > Format. Or, if you are adding a whole new file, just Source > Clean Up.

When Tom is done merging, will you cut the release and upload to ImageJ2 update site? Or should one of us do it?

Usually the component maintainer will make the releases when they're happy with the state of the project. So by default I would defer to @tomka and if you want @ctrueden or me to do the release just ping us when it's ready.

As for uploading to Fiji, we use status.imagej.net to look for which components have changes. When something has a new release, we update the version in pom-fiji and then do the upload to the Fiji update site. Curtis or I can definitely do those steps after the release is cut.

In general, another way to ping us that a release is ready for Fiji is to submit a PR to pom-fiji that bumps the component version.

I wondered if moving it to its own repository meant that it was now no longer a core component.

As long as it's in the Fiji update site it's still a core component (defined here! :)

[chalkie666]

@tomka hi there! If you don't have time to do this merge, just let me know, and i will have a go at it myself. cheers! D

[tomka]

Hi Dan,

I've started reworking your commits already and should be able to finish this today. Like I said above, I'd like to change a couple of things before I merge. For instance the failing test should live in a separate branch along with your upcoming changes that fix it and hence won't be merged right now. Most of it can be merged though.

[tomka]

I created three branches out of the changes above: general changes, job name and mask-pixels. The first branch is already available as a separate pull request (#19) and the second one will follow later when I cleaned up its commits. Both should merge without problems.

The third branch won't be merged until the feature is actually implemented, because I would like to try to keep master stable. @chalkie666, please continue development of the mask-pixel changes in this separate topic branch and create a pull request once you consider it done. I'll link the branch once I pushed it (later today). Is this okay with you?

[chalkie666]

Hi Tom

that's all super - thanks for taking the time to fix it up! Yes, I totally agree we must keep master branch clean, non broken, and passing all unit tests.

let me know if there are problems with code style etc still.... i might need a bit of hand holding .... until i get it right so you dont have to tidy up as much,

cheers

d

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

I created three branches out of the changes above: general changes, job name and mask-pixels. The first branch is already available as a separate pull request (#19 https://github.com/fiji/Colocalisation_Analysis/pull/19) and the second one will follow later when I cleaned up its commits. Both should merge without problems.

The third branch won't be merged until the feature is actually implemented, because I would like to try to keep master stable. @chalkie666 https://github.com/chalkie666, please continue development of the mask-pixel changes in this separate topic branch and create a pull request once you consider it done. I'll link the branch once I pushed it (later today). Is this okay with you?

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