search

imagej / imagej-ops

ImageJ Ops: "Write once, run anywhere" image processing
https://imagej.net/libs/imagej-ops
BSD 2-Clause "Simplified" License
90 stars 42 forks source link

Add watershed ops #510

Closed ctrueden closed 6 years ago

ctrueden commented 7 years ago

Continuation of #446, rebased over the latest master.

Still needs other comments from @dietzc addressed.

tibuch commented 7 years ago

I fixed a couple of the issues in #446.

Unfortunately the implementation has some division by zero exceptions. @SimonSchmid any ideas?

SimonSchmid commented 7 years ago

Thanks @tibuch! As far as I remember I didn't have division by zero exceptions. Does it happen only with certain input images? How can I reproduce it?

tibuch commented 7 years ago

@SimonSchmid executing the JUnit tests will throw the Exception.

SimonSchmid commented 7 years ago

@tibuch Since I don't have permissions to commit and git does not support the file type, I have sent you a patch per mail to fix the bug. Thanks again!

imagejan commented 7 years ago

@SimonSchmid wrote:

I don't have permissions to commit

You can always fork the project and do the commits on your fork SimonSchmid/imagej-ops. Then you can link to github's compare view to discuss your changes publicly.

tibuch commented 7 years ago

All tests are passing.

gselzer commented 7 years ago

@SimonSchmid, as of now there is no ImageJFunctions class in ImgLib2, so you should have to rework your tests. I do not know if anything else is out of date at this point so it might be worth testing this code with the current version of ImgLib2 to check.

I was also wondering whether or not it would be best to separate commit d2954fa2c66a85986b2a8413a78877ad858867e0 into smaller commits considering its sheer size. While it is not necessary to do so it would make the documentation a lot easier to read.

Other than the concerns presented above I think this PR is ready to be merged.

dietzc commented 7 years ago

I was also wondering whether or not it would be best to separate commit d2954fa2c66a85986b2a8413a78877ad858867e0 into smaller commits considering its sheer size. While it is not necessary to do so it would make the documentation a lot easier to read.

I don't really see the reason for that in this case, as most of the files are either new and somehow belong together. Other than that: @SimonSchmid maybe this PR is something you can tackle at the upcoming hackathon?

ctrueden commented 7 years ago

as of now there is no ImageJFunctions class in ImgLib2

@gselzer That class is part of the imglib2-ij component, which provides integration between ImgLib2 and ImageJ1.

so you should have to rework your tests

Well, do the tests work as things currently stand on this topic branch? Simple enough to try them, no?

I don't really see the reason for that in this case

I agree with @dietzc that if it's a new implementation being added, having a big commit that dumps it in is not so bad. Yes, it's a big diff, but it can also be a real pain to make smaller commits for little practical benefit. There is a balance there.

@gselzer The real questions are: A) do you like this implementation; and B) does it work?

@dietzc Just to give you some background, I asked @gselzer to review some of the pending PRs so that we can start reducing the backlog. I'm hoping he can ping me when specific PRs are good to merge, and also ping each PR's author(s) again when they have not responded to requests for revision, etc.

stelfrich commented 7 years ago

I have discussed some potential improvements with @SimonSchmid during the recent Konstanz hackathon (see commit message of https://github.com/imagej/imagej-ops/pull/510/commits/fecde124d0da36cbbdb19a123f16ee7137fcdc4d for details). He will work on integrating the improvements as time allows and I will ping him every once in a while :smile:

ctrueden commented 6 years ago

I rebased this over the latest master. All tests pass.

@stelfrich @tibuch @SimonSchmid What is the current status? I see that final WIP commit with a bunch of changes... what are the next steps to get this finished?

ctrueden commented 6 years ago

@SimonSchmid I invited you to the Ops developers team. If you work on the watershed ops any further, then please work on this branch (watershed) right here.

SimonSchmid commented 6 years ago

@ctrueden Thanks, my plan is to work next week in Dresden at this! :)

ctrueden commented 6 years ago

@stelfrich Are you happy with the most current version of this now? If so, I will ask @gselzer to rebase this over the latest master so that we can merge it.

stelfrich commented 6 years ago

Are you happy with the most current version of this now?

I am @ctrueden. But I think the history of this branch could be improved, especially b70b028 (WIP) and cf8020d. We could try to squash some of them together, since splitting them apart looks cumbersome? Is it something @gselzer would do, or do you, @SimonSchmid, have time to that?

SimonSchmid commented 6 years ago

@stelfrich Sorry, I do not really have time for this the next days/weeks, I am having exams and doing my bachelor thesis.

ctrueden commented 6 years ago

Either @gselzer or I will fix the history, and get this merged.

gselzer commented 6 years ago

@ctrueden and I went over the commits, and decided it would be most expedient to simply squash everything into one commit. Thanks everyone for your work on this!