Add single-threaded version to LocalExtrema and allow for caller specified neighborhoods. #37

hanslovsky · 2017-03-21T15:31:07Z

This commit adds:

shape parameter for callers to specify local neighborhood
Single-threaded version of findLocalExtrema
Parameter for caller to specify number of tasks.
Note in the JavaDoc to inform user that border pixels are ignored.
Relax the bounds on generic parameter T: Was Comparable< T >, now unbounded.

The original interface is preserved for a non-breaking change.

Code coverage is now at 88% for LocalExtrema.java, only missing:

Exception handlings for parallel execution
assert statements
MinimumCheck class

See also:
https://gitter.im/imglib/imglib2?at=58bd72101465c46a56f2ae9c

This PR is a rebase of and closes #33

…ified neighborhoods. This commit adds: - shape parameter for callers to specify local neighborhood - Single-threaded version of findLocalExtrema - Parameter for caller to specify number of tasks. - Note in the JavaDoc to inform user that border pixels are ignored. - Relax the bounds on generic parameter T: Was Comparable< T >, now unbounded. The original interface is preserved for a non-breaking change. Code coverage is now at 88% for LocalExtrema.java, only missing: - Exception handlings for parallel execution - assert statements - MinimumCheck class See also: https://gitter.im/imglib/imglib2?at=58bd72101465c46a56f2ae9c Add multi-threaded interface to accept RandomAccessible and Interval This way the implementation is much cleaner. The RandomAccessibleInterval interface remains unchanged. Fix bug in determining the maximum size of source Add license and author tag to LocalExtremaTest Add more comprehensive test for better code coverage. The previously existing interface was not tested. It is tested now. Code coverage is now at 88% for LocalExtrema.java, only missing: - Exception handlings for parallel execution - assert statements - MinimumCheck class

tpietzsch · 2017-03-22T12:02:40Z

src/main/java/net/imglib2/algorithm/localextrema/LocalExtrema.java

+		// TODO It is probably better to throw exception than to use try/catch
+		// block and return potentially incomplete/inconsistent list of extrema.
+		// Returning an empty list on exception could be a compromise without
+		// changing the interface.


I agree, throwing an exception is the better way!
Let's change the API, and add a deprecated version with the old signature that prints the exception and returns null.

Sounds reasonable to me! I will add that.

As we are at interface changes already, what about @tinevez suggestion in #33 :
#33 (review)
We could return a List in the new interface but cast to ArrayList before returning in the deprecated version because we know the implementation details.
I still think that we should stick with ArrayList because that is unlikely to change but I'd like to hear your opinion on it.

In the original implementation the task was split along the last dimension. Should I revert to that for the deprecated version for consistency?

Yes, sounds good. I'm for changing it to List. (or even Collection, but that's probably overdoing it...)

And yes, I would revert to splitting along the last dimension

I will change it to List, then. And revert to spliiting along the last dimension for the deprecated version.

tpietzsch · 2017-03-22T12:05:50Z

src/main/java/net/imglib2/algorithm/localextrema/LocalExtrema.java

+	{
+		final RandomAccessible< Neighborhood< Object > > neighborhood = shape.neighborhoodsRandomAccessible( ConstantUtils.constantRandomAccessible( new Object(), nDim ) );
+		final long[] min = LongStream.generate( () -> Long.MAX_VALUE ).limit( nDim ).toArray();
+		final long[] max = LongStream.generate( () -> Long.MIN_VALUE ).limit( nDim ).toArray();


I probably sound like a grumpy old man here, but what's wrong with Arrays.fill(max, Long.MIN_VALUE)?
:-)

I am using streams because Java still lacks a function that generates an array that's filled with a constant value in a single line, something like Arrays.create( int length, long value ) would be great. This is my little workaround for that. ;) At least for me, readability is improved wthat way.

tpietzsch · 2017-03-22T12:08:53Z

src/main/java/net/imglib2/algorithm/localextrema/LocalExtrema.java

+			max[ d ] = Math.max( bb.max( d ), max[ d ] );
+		}
+
+		final long[] borderSize = IntStream.range( 0, nDim ).mapToLong( d -> Math.max( max[ d ], -min[ d ] ) ).toArray();


Similar here. Why not simply put borderSize[ d ] = Math.max( max[ d ], -min[ d ] ); into the above for loop? To me it looks more readable, but that is of course a personal preference...

(Also it's probably much faster, but that isn't important here)

Again, I prefer the one-liner but that might be because I am coming from the python/numpy world originally where I would do something like numpy.maximum( max, -min )

tpietzsch · 2017-03-22T12:23:02Z

src/main/java/net/imglib2/algorithm/localextrema/LocalExtrema.java

+		final long[] borderSize = getRequiredBorderSize( shape, source.numDimensions() );
+		final int nDim = source.numDimensions();
+		// Get biggest dimension after border subtraction. Parallelize along
+		// this dimension.


I'm not sure this is always a good choice. Assuming some kind of flattened array, where the highest dimension is the slowest changing, it is probably preferable to choose that one because of cache utilisation (even if the extent is larger along another dimension). It would be interesting to see a benchmark.

In the end, of course it can be a rotated view or the highest dimension may have extent 1, etc...

Can you add a version where the parallelisation dimension is a parameter?

Good point, I will add that and this method should delegate to it!

@deprecated

- Add @deprecated to deprecated method - Revert to parallelizing over last dimension for deprecated method - Add method signatures with split dimension as parameter - Return `List` instead of `ArrayList` (deprecated method still returns `ArrayList`) The test cases have been made more comprehensive and miss only assertion statements and exception handling in the deprecated method.

hanslovsky · 2017-11-16T14:38:19Z

Can this be merged now?

tpietzsch · 2017-11-16T15:40:51Z

src/main/java/net/imglib2/algorithm/localextrema/LocalExtrema.java

+
+		assert Arrays.stream( borderSize ).min().getAsLong() >= 0: "Border size cannot be smaller than zero.";
+
+		// TODO use Intervals.expand once it is available in non-SNAPSHOT


I just released imglib2-4.6.0. The Intervals.expand should be now available.

tpietzsch · 2017-11-16T15:42:33Z

I just released imglib2-4.6.0. The Intervals.expand should be now available. Could you address that one TODO item referring to that?

Then it's ready to go!

(There is a javadoc link error in the class comment. It was there already before, but maybe while you're at it, could you fix it?)

hanslovsky · 2017-11-16T16:03:20Z

Great, I'll take care of the Intervals.expand and the javadoc link error.

Update:
I modified the JavaDoc but I realized that, in order for us to use code from imglib2-4.6.0, we would either override the managed version, or update and release pom-imglib/pom-scijava with imglib2.version set to 4.6.0. I think that none of these options are feasible. Instead, we should

merge as is, or
wait until the updates to pom-imglib/pom-scijava happen
I am fine with either of those.

axtimwalde · 2017-11-16T19:16:26Z

I think that overriding the one dependency version that you care for is ok?

hanslovsky · 2017-11-16T21:23:55Z

If that is the general policy, I can of course go ahead and add the required changes. Is that ok only while on a SNAPSHOT version, i.e. overriding the dependency version should not happen in a release version?

ctrueden · 2017-11-16T21:45:28Z

Just to describe the general SciJava policy: for projects in scijava, imagej, scifio and fiji, the policy is that all commits merging to master must reproducibly compile with passing tests—otherwise it is a "bug". So that means you cannot have a SNAPSHOT dependency in any such commits, even if a later commit "fixes" it. Any commit adding a SNAPSHOT dependency should be labeled as WIP and fixed (i.e. rebased) to not be a WIP before that branch can be merged.

There is a script to facilitate meeting this requirement here.

Personally I would prefer and recommend that imglib projects also have this requirement, but ultimately that is up to @tpietzsch, @axtimwalde and @StephanPreibisch.

update and release pom-imglib/pom-scijava with imglib2.version set to 4.6.0. I think that none of these options are feasible.

If I understand @axtimwalde's suggestion correctly, you can just override <imglib2.version>4.6.0</imglib2.version> in the <properties> of the POM here, and it meets the requirement I outlined above. That is: there is no requirement that you never override a component version. In general I'd discourage it, since it can result in version skew, but the build will still be reproducible at least. It's good for such pins to be transient—i.e., to try to eliminate them over time in favor of pom-scijava updates.

Is that ok only while on a SNAPSHOT version, i.e. overriding the dependency version should not happen in a release version?

That seems like a reasonable rule of thumb. That is: when a project is about to be released, we make an effort to eliminate the pinned version properties. However, I think there are cases (e.g., a bunch of components in a dependency chain, where work was done at the lowest levels that needs to propagate downstream) where it can be burdensome. So I'd hesitate to make this a hard requirement.

hanslovsky · 2017-11-17T15:23:17Z

Thanks for the clarification @ctrueden
The reason why I asked was exactly that version skew that you were referring to in your post but that does not seem to be a big concern here, so I will go ahead and update my branch with the suggested changes @tpietzsch @axtimwalde

ctrueden · 2017-11-17T17:15:57Z

version skew that you were referring to in your post but that does not seem to be a big concern here

It is not a concern here. Rather, the problems typically happen downstream somewhere. For example, suppose I have a project which extends the latest pom-scijava, but wants to use this new version of LocalExtrema you've added here. So I pin to the latest release of imglib2-algorithm, leaving the rest of the versions as default inherited from pom-scijava. Now my code will fail at runtime, because it will use a too-old version of imglib2. This will be surprising to many people, and in some cases very subtle and dangerous when, instead of an explicit API breakage/mismatch, we instead inherit some old version of a component with a subtle but devastating bug that is not easily noticed by a human.

In my experience, the easiest way to avoid these kinds of problems is to keep pom-scijava as up-to-date as possible, and keep our component stack extending the latest version of pom-scijava in favor of hardcoding pinned versions. It's not a perfect solution in every case, but it certainly makes a big difference in the frequency of these sorts of scenarios.

This is, BTW, one big reason why I think maintaining a separate pom-imglib2 parent is not a good idea.

hanslovsky · 2018-01-11T17:06:02Z

I saw that pom-imglib2-10.0.1 was released that has pom-scijava-19.0.0 as parent, which defines <imglib2.version>4.6.0</imglib2.version>. Can we update the parent version of imglib2-algorithm to 10.0.1 so we can merge this without overriding imglib2.version and avoiding potential version skew?

hanslovsky · 2018-01-19T19:43:42Z

This happened in #50 I merged master into this branch so this should be ready to be merged now without version skew.

hanslovsky · 2018-01-29T20:49:15Z

@imglib/admins can we merge this? From the discussion it seems that the imglib2 dependency version was the only remaining issue, which seems to be resolved now. I do not want to merge my own PR (as a non-owner), though.

axtimwalde · 2018-01-29T22:21:47Z

Pleas merge.

tpietzsch reviewed Mar 22, 2017

View reviewed changes

tpietzsch reviewed Nov 16, 2017

View reviewed changes

hanslovsky added 2 commits November 16, 2017 11:16

Merge branch 'master' into local-extrema-rebase

82dcba8

Fix JavaDoc

bbd9383

Update imglib2 dependency to 4.6.0 so we can use Views.expand

ce975ce

Merge remote-tracking branch 'i/master' into local-extrema-rebase

842e16d

hanslovsky merged commit bea8391 into imglib:master Jan 30, 2018


		assert Arrays.stream( borderSize ).min().getAsLong() >= 0: "Border size cannot be smaller than zero.";

		// TODO use Intervals.expand once it is available in non-SNAPSHOT

Add single-threaded version to LocalExtrema and allow for caller specified neighborhoods. #37

Add single-threaded version to LocalExtrema and allow for caller specified neighborhoods. #37

Uh oh!

Conversation

hanslovsky commented Mar 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanslovsky Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanslovsky commented Nov 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tpietzsch commented Nov 16, 2017

Uh oh!

hanslovsky commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

axtimwalde commented Nov 16, 2017

Uh oh!

hanslovsky commented Nov 16, 2017

Uh oh!

ctrueden commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hanslovsky commented Nov 17, 2017

Uh oh!

ctrueden commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hanslovsky commented Jan 11, 2018

Uh oh!

hanslovsky commented Jan 19, 2018

Uh oh!

hanslovsky commented Jan 29, 2018

Uh oh!

axtimwalde commented Jan 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hanslovsky Mar 22, 2017 •

edited

Loading

hanslovsky commented Nov 16, 2017 •

edited

Loading

ctrueden commented Nov 16, 2017 •

edited

Loading

ctrueden commented Nov 17, 2017 •

edited

Loading