New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add watershed ops #510

Merged
merged 14 commits into from Feb 19, 2018

Conversation

Projects
None yet
7 participants
@ctrueden
Member

ctrueden commented Jun 22, 2017

Continuation of #446, rebased over the latest master.

Still needs other comments from @dietzc addressed.

@tibuch

This comment has been minimized.

Show comment
Hide comment
@tibuch

tibuch Jun 23, 2017

Contributor

I fixed a couple of the issues in #446.

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

Contributor

tibuch commented Jun 23, 2017

I fixed a couple of the issues in #446.

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

@SimonSchmid

This comment has been minimized.

Show comment
Hide comment
@SimonSchmid

SimonSchmid Jun 26, 2017

Contributor

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?

Contributor

SimonSchmid commented Jun 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@tibuch

tibuch Jun 26, 2017

Contributor

@SimonSchmid executing the JUnit tests will throw the Exception.

Contributor

tibuch commented Jun 26, 2017

@SimonSchmid executing the JUnit tests will throw the Exception.

@SimonSchmid

This comment has been minimized.

Show comment
Hide comment
@SimonSchmid

SimonSchmid Jun 29, 2017

Contributor

@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!

Contributor

SimonSchmid commented Jun 29, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@imagejan

imagejan Jun 30, 2017

Member

@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.

Member

imagejan commented Jun 30, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@tibuch

tibuch Jun 30, 2017

Contributor

All tests are passing.

Contributor

tibuch commented Jun 30, 2017

All tests are passing.

@tibuch tibuch requested a review from dietzc Jun 30, 2017

@gselzer

This comment has been minimized.

Show comment
Hide comment
@gselzer

gselzer Aug 22, 2017

Contributor

@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.

Contributor

gselzer commented Aug 22, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@dietzc

dietzc Aug 22, 2017

Member

I was also wondering whether or not it would be best to separate commit d2954fa 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?

Member

dietzc commented Aug 22, 2017

I was also wondering whether or not it would be best to separate commit d2954fa 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

This comment has been minimized.

Show comment
Hide comment
@ctrueden

ctrueden Aug 23, 2017

Member

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.

Member

ctrueden commented Aug 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@stelfrich

stelfrich Oct 4, 2017

Member

I have discussed some potential improvements with @SimonSchmid during the recent Konstanz hackathon (see commit message of fecde12 for details). He will work on integrating the improvements as time allows and I will ping him every once in a while 😄

Member

stelfrich commented Oct 4, 2017

I have discussed some potential improvements with @SimonSchmid during the recent Konstanz hackathon (see commit message of fecde12 for details). He will work on integrating the improvements as time allows and I will ping him every once in a while 😄

@ctrueden

This comment has been minimized.

Show comment
Hide comment
@ctrueden

ctrueden Dec 5, 2017

Member

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?

Member

ctrueden commented Dec 5, 2017

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 ctrueden referenced this pull request Dec 5, 2017

Closed

Watershed Ops added #446

@ctrueden

This comment has been minimized.

Show comment
Hide comment
@ctrueden

ctrueden Dec 5, 2017

Member

@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.

Member

ctrueden commented Dec 5, 2017

@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.

@ctrueden ctrueden changed the title from WIP: Add watershed ops to Add watershed ops Dec 5, 2017

@SimonSchmid

This comment has been minimized.

Show comment
Hide comment
@SimonSchmid

SimonSchmid Dec 5, 2017

Contributor

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

Contributor

SimonSchmid commented Dec 5, 2017

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

@stelfrich

Thanks for working on this @SimonSchmid! The implementation of the algorithm looks sound to me. I have made some comments, that need to be discussed before merging this. The most prominent one being how we want to handle non-empty out() (in conjunction with a provided mask).

Show outdated Hide outdated src/main/java/net/imagej/ops/image/ImageNamespace.java Outdated
Show outdated Hide outdated src/main/java/net/imagej/ops/image/ImageNamespace.java Outdated
Show outdated Hide outdated src/main/java/net/imagej/ops/image/watershed/Watershed.java Outdated
Show outdated Hide outdated src/main/java/net/imagej/ops/image/watershed/Watershed.java Outdated
Show outdated Hide outdated src/main/java/net/imagej/ops/image/watershed/Watershed.java Outdated
Show outdated Hide outdated src/main/java/net/imagej/ops/image/watershed/WatershedSeeded.java Outdated
}
/*
* Merge already present labels before calculation of watershed

This comment has been minimized.

@stelfrich

stelfrich Dec 15, 2017

Member

Same thought as in Watershed applies here

@stelfrich

stelfrich Dec 15, 2017

Member

Same thought as in Watershed applies here

@Override
public boolean conforms() {
boolean conformed = true;

This comment has been minimized.

@stelfrich

stelfrich Dec 15, 2017

Member

I suggest that we require mask and seeds to have the same dimensionality as in()

@stelfrich

stelfrich Dec 15, 2017

Member

I suggest that we require mask and seeds to have the same dimensionality as in()

Show outdated Hide outdated src/main/java/net/imagej/ops/image/watershed/WatershedSeeded.java Outdated
Show outdated Hide outdated src/main/java/net/imagej/ops/image/watershed/WatershedSeeded.java Outdated
@ctrueden

This comment has been minimized.

Show comment
Hide comment
@ctrueden

ctrueden Jan 23, 2018

Member

@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.

Member

ctrueden commented Jan 23, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@stelfrich

stelfrich Jan 24, 2018

Member

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?

Member

stelfrich commented Jan 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@SimonSchmid

SimonSchmid Jan 25, 2018

Contributor

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

Contributor

SimonSchmid commented Jan 25, 2018

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

@ctrueden

This comment has been minimized.

Show comment
Hide comment
@ctrueden

ctrueden Jan 25, 2018

Member

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

Member

ctrueden commented Jan 25, 2018

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

tibuch and others added some commits Jun 23, 2017

WIP
Update SCIFIO dependency to 0.33.0. 0.32.0 had some concurrency issues.

TODOs:
- the implementation should only change labels for the pixels that are
  defined by the mask (if one is provided); I have adapted the tests to
  only compare outcomes within the region, so there is currently no
  failing test for that requirement
- if a seed image contains pixels with more than one label cancel the
  computation because we cannot determine which one to start with
- replace the Intervals.contains checks with extending the input image
  with a dedicated BOUNDARY label (maybe?)
- it might be worth checking to see if we can get around using both the
  output labeling and currentLabeling by merging DUMMY and WATERSHED
  labels into currentLabeling; that way we can use the mask again to
  copy over the computed results at the end of the computation
Finish watershed ops
* Resolved Stephans TODOs
* Adapted namespace
* Adapted tests

@gselzer gselzer merged commit 1375719 into master Feb 19, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@gselzer

This comment has been minimized.

Show comment
Hide comment
@gselzer

gselzer Feb 19, 2018

Contributor

@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!

Contributor

gselzer commented Feb 19, 2018

@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!

@gselzer gselzer deleted the watershed branch Feb 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment