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 additional ROI shapes and functionality #29

Merged
merged 179 commits into from Feb 13, 2018

Conversation

Projects
None yet
6 participants
@awalter17
Contributor

awalter17 commented Jun 26, 2017

Hello!

This branch includes several changes to the ROI API. The "major" changes are:

  • Changes Contains to Mask which is the base for all ROIs
  • Adds additional real space ROI shapes
    • Implementations which include points on boundary (closed), excludes points on boundary (open), and undefined behavior
  • Adds operations for combining/transforming ROIs
    • Earlier in the branch this is done one way, and then at 37df63b this setup is reworked to be functions
  • Adds adapters for going between (Real)RandomAccessibles and Masks
  • Adds interfaces for ROIs which know their bounds

Some of the changes on this branch are based on the work done in #25.

All the commits on this branch compile with passing tests. @tpietzsch please let me know your thoughts on these changes or if you have any questions.

Thank you very much,

Alison

P.S.: The demos which previously existed on this branch have been moved to roi-demo. These demos require a circular dependency on bdv-core, they'll probably be moved elsewhere at a later date.

@awalter17

This comment has been minimized.

Show comment
Hide comment
@awalter17

awalter17 Aug 28, 2017

Contributor

@tpietzsch Have you had an opportunity to review these changes? If not that's fine, but if you could please give me a rough approximation of when you may have the time to review them I would greatly appreciate it.

Contributor

awalter17 commented Aug 28, 2017

@tpietzsch Have you had an opportunity to review these changes? If not that's fine, but if you could please give me a rough approximation of when you may have the time to review them I would greatly appreciate it.

@tpietzsch

This comment has been minimized.

Show comment
Hide comment
@tpietzsch

tpietzsch Aug 31, 2017

Member

@awalter17 I haven't looked at it yet, but @ctrueden gave a quick demo at the learnathon and it looked fine. I'll review and merge (most likely) during the upcoming Konstanz hackathon

Member

tpietzsch commented Aug 31, 2017

@awalter17 I haven't looked at it yet, but @ctrueden gave a quick demo at the learnathon and it looked fine. I'll review and merge (most likely) during the upcoming Konstanz hackathon

@tpietzsch

This comment has been minimized.

Show comment
Hide comment
@tpietzsch

tpietzsch Oct 2, 2017

Member

@awalter17 I looked into it during the Konstanz hackathon as promised. There were a few rough edges and basically I started pulling on a thread and it all came apart... :-)
You had Mask extend java.util.function.Predicate (which I like). Predicate has default and(), or(), and negate() methods and it would be reasonable to assume that these do the right thing in Mask (produce another mask). I tried to bend the current PR into that shape but it didn't really work.

There is a complete parallel re-implementation of the core interfaces and all the operator logic in package net.imglib2.troi in this branch. What is missing is mainly the adaption of all concrete shapes to these interfaces, and transform operations. For getting started, have a look at net.imglib2.troi.Demo which recapitulates the testAnd() method of your Demo. It has a lot of comments of what is changed with respect to the old demo.
Also there is some javadoc on the re-implementation.

Some notes:

  • A decision that was causing problems in your design was to let apply-transform-to-mask be a binary operator which has a mask and a transform as operands. This should be formulated as a (parametrised) unary operator which only has the mask as an operand. This allows for tighter typing of operations results, composite masks, etc.
  • The mask operations (OR, AND, ...) are now cleany separated into operations on the BoundaryType (implemented here), on the Bounds (here and here, more on that below), and on the Predicate function (implemented here). There is of course boilerplate to support it, but the core logic for e.g. AND is implemented only once and applies to integral/real/interval/unbounded masks.
  • There is now the distinction between real/integer Mask and RealMask interfaces. Before, the Mask<T> was that applied to Localizable or RealLocalizable and then at MaskInterval/MaskRealInterval the hierarchy separated into real/integer. I think that was the main reason why correct implementation of the Predicate operators wasn't possible with the old design.
  • The Bounds class encapsulates reasoning about mask boundaries. There are basically too options: A mask can have an interval (outside of which it always evaluates to false) or be unbounded. In the first case, we need special consideration of empty intervals (that can arise for example by ANDing two disjoint MaskIntervals). Empty intervals are recognised by having max<min in at least one dimension. To make sure that these empty intervals union/intersect correctly with other intervals, we report min=POSITIVE_INFINITY/max=NEGATIVE_INFINITY in all dimensions as soon as an interval becomes empty.
  • Also, we need to consider changing intervals on masks. I.e. if you have mask1, mask2 and mask1 AND mask2, then if you shift mask1 around, the interval on mask1 AND mask2 needs to change. The alternative would be having only unmodifiable masks. After discussion with @ctrueden, this is not the way we want to go. So we need intervals that combine constituent intervals and update their min/max/emptiness as these constituents are modified. All this is taken care of by inner classes in the Bounds class.
  • Fortunately, modifying constituent intervals can never make a (potentially empty) interval become unbounded. Therefore, the interval/unbounded decision can be expressed into argument/return types of the Mask.and() etc methods. For example, consider:
interface Mask extends Predicate<Localizable> ... {
	@Override
	default Mask and( Predicate<? super Localizable> other ) { ... }

	@Override
	default Mask or( Predicate<? super Localizable> other ) { ... }
}

interface MaskInterval extends Mask ... {
	@Override
	default MaskInterval and( Predicate< ? super Localizable > other ) { ... }

	// *NOT* overriding Mask.or(), just specializing for MaskInterval argument.
	default MaskInterval or( MaskInterval other ) { ... }

	...
}

MaskInterval.and(anything) always results in another MaskInterval. For MaskInterval.or(anything) it depends on whether anything is an interval or not. This is very convenient an avoids unnecessary casting (see Demo).

  • Playing with Sphere/Box for the demo, I found the double[]-centered-ness a bit annoying. Maybe we could try to be more imglib-consistent there and allow for Localizable/Positionable where appropriate. For exampe, if the Sphere.center() would simply give me a RealLocalizable & RealPositionable that I can use to move/ask for the sphere's position, that would be convenient. (But I haven't thought that through. Just something I noticed.)
  • In your code, you use transformFromSource. I would prefer transformToSource, otherwise we are restricted to invertible transformations. For example, taking a 2D slice out of a 3D mask is not possible with transformFromSource.

There is a second demo ModifyDemo.java that showcases the composite with modifiable parts...
screen shot 2017-10-03 at 01 09 05
Three spheres that you can move around and the composite RealMaskRealInterval composite = s1.and( s2 ).and( s3 ); mask (bright yellow). The interval of the composite is overlaid as a red box (or the text "composite interval is empty").

So, the next TODOs would be

  • adapt concrete geom regions to the new interfaces.
  • implement transforms (maybe affine on RealMask is enough for now) for the new stuff. Consider transformFromSource vs transformToSource!
  • throw out the old implementation and move package net.imglib2.troi to net.imglib2.roi.

After that, it would be nice if somebody else (@axtimwalde?) could also do a review because I'm rather biased towards my solution now ... :)

Member

tpietzsch commented Oct 2, 2017

@awalter17 I looked into it during the Konstanz hackathon as promised. There were a few rough edges and basically I started pulling on a thread and it all came apart... :-)
You had Mask extend java.util.function.Predicate (which I like). Predicate has default and(), or(), and negate() methods and it would be reasonable to assume that these do the right thing in Mask (produce another mask). I tried to bend the current PR into that shape but it didn't really work.

There is a complete parallel re-implementation of the core interfaces and all the operator logic in package net.imglib2.troi in this branch. What is missing is mainly the adaption of all concrete shapes to these interfaces, and transform operations. For getting started, have a look at net.imglib2.troi.Demo which recapitulates the testAnd() method of your Demo. It has a lot of comments of what is changed with respect to the old demo.
Also there is some javadoc on the re-implementation.

Some notes:

  • A decision that was causing problems in your design was to let apply-transform-to-mask be a binary operator which has a mask and a transform as operands. This should be formulated as a (parametrised) unary operator which only has the mask as an operand. This allows for tighter typing of operations results, composite masks, etc.
  • The mask operations (OR, AND, ...) are now cleany separated into operations on the BoundaryType (implemented here), on the Bounds (here and here, more on that below), and on the Predicate function (implemented here). There is of course boilerplate to support it, but the core logic for e.g. AND is implemented only once and applies to integral/real/interval/unbounded masks.
  • There is now the distinction between real/integer Mask and RealMask interfaces. Before, the Mask<T> was that applied to Localizable or RealLocalizable and then at MaskInterval/MaskRealInterval the hierarchy separated into real/integer. I think that was the main reason why correct implementation of the Predicate operators wasn't possible with the old design.
  • The Bounds class encapsulates reasoning about mask boundaries. There are basically too options: A mask can have an interval (outside of which it always evaluates to false) or be unbounded. In the first case, we need special consideration of empty intervals (that can arise for example by ANDing two disjoint MaskIntervals). Empty intervals are recognised by having max<min in at least one dimension. To make sure that these empty intervals union/intersect correctly with other intervals, we report min=POSITIVE_INFINITY/max=NEGATIVE_INFINITY in all dimensions as soon as an interval becomes empty.
  • Also, we need to consider changing intervals on masks. I.e. if you have mask1, mask2 and mask1 AND mask2, then if you shift mask1 around, the interval on mask1 AND mask2 needs to change. The alternative would be having only unmodifiable masks. After discussion with @ctrueden, this is not the way we want to go. So we need intervals that combine constituent intervals and update their min/max/emptiness as these constituents are modified. All this is taken care of by inner classes in the Bounds class.
  • Fortunately, modifying constituent intervals can never make a (potentially empty) interval become unbounded. Therefore, the interval/unbounded decision can be expressed into argument/return types of the Mask.and() etc methods. For example, consider:
interface Mask extends Predicate<Localizable> ... {
	@Override
	default Mask and( Predicate<? super Localizable> other ) { ... }

	@Override
	default Mask or( Predicate<? super Localizable> other ) { ... }
}

interface MaskInterval extends Mask ... {
	@Override
	default MaskInterval and( Predicate< ? super Localizable > other ) { ... }

	// *NOT* overriding Mask.or(), just specializing for MaskInterval argument.
	default MaskInterval or( MaskInterval other ) { ... }

	...
}

MaskInterval.and(anything) always results in another MaskInterval. For MaskInterval.or(anything) it depends on whether anything is an interval or not. This is very convenient an avoids unnecessary casting (see Demo).

  • Playing with Sphere/Box for the demo, I found the double[]-centered-ness a bit annoying. Maybe we could try to be more imglib-consistent there and allow for Localizable/Positionable where appropriate. For exampe, if the Sphere.center() would simply give me a RealLocalizable & RealPositionable that I can use to move/ask for the sphere's position, that would be convenient. (But I haven't thought that through. Just something I noticed.)
  • In your code, you use transformFromSource. I would prefer transformToSource, otherwise we are restricted to invertible transformations. For example, taking a 2D slice out of a 3D mask is not possible with transformFromSource.

There is a second demo ModifyDemo.java that showcases the composite with modifiable parts...
screen shot 2017-10-03 at 01 09 05
Three spheres that you can move around and the composite RealMaskRealInterval composite = s1.and( s2 ).and( s3 ); mask (bright yellow). The interval of the composite is overlaid as a red box (or the text "composite interval is empty").

So, the next TODOs would be

  • adapt concrete geom regions to the new interfaces.
  • implement transforms (maybe affine on RealMask is enough for now) for the new stuff. Consider transformFromSource vs transformToSource!
  • throw out the old implementation and move package net.imglib2.troi to net.imglib2.roi.

After that, it would be nice if somebody else (@axtimwalde?) could also do a review because I'm rather biased towards my solution now ... :)

@awalter17

This comment has been minimized.

Show comment
Hide comment
@awalter17

awalter17 Oct 3, 2017

Contributor

Thanks @tpietzsch! I have been looking over your changes, and I think they are a big improvement.

I did have one question regarding retrieving operands and operation type (and, or, xor, etc.) from a MaskPredicate which resulted from an operation on MaskPredicate(s). I posted about this in more detail on the forum. If you could please let me know your thoughts on this, I'd greatly appreciate it.

Contributor

awalter17 commented Oct 3, 2017

Thanks @tpietzsch! I have been looking over your changes, and I think they are a big improvement.

I did have one question regarding retrieving operands and operation type (and, or, xor, etc.) from a MaskPredicate which resulted from an operation on MaskPredicate(s). I posted about this in more detail on the forum. If you could please let me know your thoughts on this, I'd greatly appreciate it.

@tpietzsch

This comment has been minimized.

Show comment
Hide comment
@tpietzsch

tpietzsch Oct 4, 2017

Member

@awalter17 I replied on the forum

Member

tpietzsch commented Oct 4, 2017

@awalter17 I replied on the forum

@awalter17

This comment has been minimized.

Show comment
Hide comment
@awalter17

awalter17 Oct 6, 2017

Contributor

Playing with Sphere/Box for the demo, I found the double[]-centered-ness a bit annoying. Maybe we could try to be more imglib-consistent there and allow for Localizable/Positionable where appropriate. For exampe, if the Sphere.center() would simply give me a RealLocalizable & RealPositionable that I can use to move/ask for the sphere's position, that would be convenient. (But I haven't thought that through. Just something I noticed.)

Initially, I was very careful to ensure that the underlying double[] was never exposed and couldn’t be mutated without using setCenter(...). I also kept the geom regions from being positionable since there was already PositionableIterableRegion. So my intuition was that the concrete geom regions should not be positionable, and instead if the region needed to be positioned Regions.positionable(...) could be used (though it would need to be Iterable first). That being said, I don’t really have strong feelings about this decision.

If I change the center() methods to return a RealLocalizable & RealPositionable, should I just have the geom regions themselves implement RealPositionable? Since they’d essentially be positionable anyway (i.e. Sphere.center().move(...) would be the same as Sphere.move(...)).

I could make a RealPositionableRegion interface and have the geom regions which implement center() implement that interface. I don’t think every geom region needs to be positionable though (i.e RealPointCollection, transformed MaskPredicate, etc.).

Please let me know your thoughts on this @tpietzsch.

Contributor

awalter17 commented Oct 6, 2017

Playing with Sphere/Box for the demo, I found the double[]-centered-ness a bit annoying. Maybe we could try to be more imglib-consistent there and allow for Localizable/Positionable where appropriate. For exampe, if the Sphere.center() would simply give me a RealLocalizable & RealPositionable that I can use to move/ask for the sphere's position, that would be convenient. (But I haven't thought that through. Just something I noticed.)

Initially, I was very careful to ensure that the underlying double[] was never exposed and couldn’t be mutated without using setCenter(...). I also kept the geom regions from being positionable since there was already PositionableIterableRegion. So my intuition was that the concrete geom regions should not be positionable, and instead if the region needed to be positioned Regions.positionable(...) could be used (though it would need to be Iterable first). That being said, I don’t really have strong feelings about this decision.

If I change the center() methods to return a RealLocalizable & RealPositionable, should I just have the geom regions themselves implement RealPositionable? Since they’d essentially be positionable anyway (i.e. Sphere.center().move(...) would be the same as Sphere.move(...)).

I could make a RealPositionableRegion interface and have the geom regions which implement center() implement that interface. I don’t think every geom region needs to be positionable though (i.e RealPointCollection, transformed MaskPredicate, etc.).

Please let me know your thoughts on this @tpietzsch.

@tpietzsch

This comment has been minimized.

Show comment
Hide comment
@tpietzsch

tpietzsch Oct 9, 2017

Member

@awalter17 I agree with your initial intuition: I would not make the geom regions themselves RealPositionable/RealLocalizable. Mainly because it is unclear where the position is anchored, saying Sphere.center().setPosition() leaves no doubt while the meaning of Sphere.setPosition() is not so clear.

Member

tpietzsch commented Oct 9, 2017

@awalter17 I agree with your initial intuition: I would not make the geom regions themselves RealPositionable/RealLocalizable. Mainly because it is unclear where the position is anchored, saying Sphere.center().setPosition() leaves no doubt while the meaning of Sphere.setPosition() is not so clear.

awalter17 added some commits Mar 20, 2017

Delete Rasterized classes and PointCollection
PointCollection will be replaced by a real space equivalent, and
rasterizing ROIs will be handled differently.
Delete ROIUtils and move methods to Regions and Labelings
Labelings is a new class for label util methods.
Rename Polygon to Polygon2D
This implementation is only for 2D polygons and should be named
accordingly.
Modify Polygon2D to not implement Mask or RRARI
ROI shapes will no longer be RRARI, instead there will be an adapter to
convert them to RealRandomAccessibles. Eventually, all ROI shapes will
be Masks.
Modify Polygon2D to use two TDoubleArrayLists
It is more efficient to use two TDoubleArrayLists than List< ? extends
RealLocalizable >. Additionally, the TDoubleArrayLists are easily
mutable.
Add implementation for open/closed Boxes and tests
This was partially based on work done by Robert Haase, see
#25.
Add open/closed implementations of SuperEllipsoid and tests
This was partially based on work done by Robert Haase, see
#25.
Delete Polygon2D tests
Updated tests will be added back in later.
Rename Polygon2D to DefaultPolygon2D
Polygon2D will be the name of the interface for all 2D Polygons, and
this will now be the default implementation.
@awalter17

This comment has been minimized.

Show comment
Hide comment
@awalter17

awalter17 Jan 18, 2018

Contributor

Make a RealLocalizableRealPositionable combination interface, and have the center() etc return that.

I created the RealLocalizableRealPositionable interface, is that an alright name for it? I'm not sure what else to call it.

Make a convenience wrapper, where you can put it a RealLocalizable & RealPositionable and it is wrapped as a RealLocalizableRealPositionable.

I've created the wrapper as well, is this what you had in mind?

Currently, I've switched the concrete implementations (ClosedBox, etc.) to use the new interface, but I still have the interfaces (Box, etc.) with <T extends RealLocalizable & RealPositionable>. Do we want these interfaces to have just <T> or should it be something like <T extends RealLocalizable>? It seems odd to have acenter() that isn't at least RealLocalizable, but there could be use cases I'm not thinking of.

Additionally, do we want the additional interfaces I suggested? So Box<T> and MutableBox which would have any setters. If so should it be named, MutableBox, WritableBox, or something else? And should my concrete implementations then contain mutable/writable i.e. ClosedMutableBox?

Thoughts @tpietzsch?

Contributor

awalter17 commented Jan 18, 2018

Make a RealLocalizableRealPositionable combination interface, and have the center() etc return that.

I created the RealLocalizableRealPositionable interface, is that an alright name for it? I'm not sure what else to call it.

Make a convenience wrapper, where you can put it a RealLocalizable & RealPositionable and it is wrapped as a RealLocalizableRealPositionable.

I've created the wrapper as well, is this what you had in mind?

Currently, I've switched the concrete implementations (ClosedBox, etc.) to use the new interface, but I still have the interfaces (Box, etc.) with <T extends RealLocalizable & RealPositionable>. Do we want these interfaces to have just <T> or should it be something like <T extends RealLocalizable>? It seems odd to have acenter() that isn't at least RealLocalizable, but there could be use cases I'm not thinking of.

Additionally, do we want the additional interfaces I suggested? So Box<T> and MutableBox which would have any setters. If so should it be named, MutableBox, WritableBox, or something else? And should my concrete implementations then contain mutable/writable i.e. ClosedMutableBox?

Thoughts @tpietzsch?

@awalter17

This comment has been minimized.

Show comment
Hide comment
@awalter17

awalter17 Jan 25, 2018

Contributor

@tpietzsch I made the remaining changes. I left RealLocalizableRealPositionable with the same name, since I couldn't think of a better one. And then for all the mutable/writable classes/interfaces I used "Writable".

This writable structure is a little odd for WritableRealPointCollection and WritablePointMask. WritableRealPointCollection still has a generic to define what is in the collection, but since its intended to be mutable I have the objects in the collection as RealLocalizable & RealPositionable. I'm not sure if this is necessary. The writable layer is nice though, since technically KDTreeRealPointCollection has never been writable so now its just a RealPointCollection while the others are WritableRealPointCollections.

For WritablePointMask, PointMask never had generics. So for it to be writable, I just have the interface extend RealPositionable directly.

If you'd prefer not to have this whole "writable" layer, we can just get rid of all commits after 382365f.

Please feel free to let me know if you'd like any changes, or if you have any questions.

Contributor

awalter17 commented Jan 25, 2018

@tpietzsch I made the remaining changes. I left RealLocalizableRealPositionable with the same name, since I couldn't think of a better one. And then for all the mutable/writable classes/interfaces I used "Writable".

This writable structure is a little odd for WritableRealPointCollection and WritablePointMask. WritableRealPointCollection still has a generic to define what is in the collection, but since its intended to be mutable I have the objects in the collection as RealLocalizable & RealPositionable. I'm not sure if this is necessary. The writable layer is nice though, since technically KDTreeRealPointCollection has never been writable so now its just a RealPointCollection while the others are WritableRealPointCollections.

For WritablePointMask, PointMask never had generics. So for it to be writable, I just have the interface extend RealPositionable directly.

If you'd prefer not to have this whole "writable" layer, we can just get rid of all commits after 382365f.

Please feel free to let me know if you'd like any changes, or if you have any questions.

@awalter17

This comment has been minimized.

Show comment
Hide comment
@awalter17

awalter17 Feb 6, 2018

Contributor

@tpietzsch Have you had the opportunity to review these changes? If not, do you think you’ll be able to review them in the next couple of weeks?

Please feel free to let me know if there’s anything I can do to help.

Contributor

awalter17 commented Feb 6, 2018

@tpietzsch Have you had the opportunity to review these changes? If not, do you think you’ll be able to review them in the next couple of weeks?

Please feel free to let me know if there’s anything I can do to help.

@tpietzsch

This comment has been minimized.

Show comment
Hide comment
@tpietzsch

tpietzsch Feb 6, 2018

Member

@awalter17 I'll do it tomorrow. Promised!

Member

tpietzsch commented Feb 6, 2018

@awalter17 I'll do it tomorrow. Promised!

tpietzsch added some commits Feb 7, 2018

Make RealLocalizableRealPositionableWrapper generic.
It would be used like this:

RealLocalizableRealPositionable p
	= new RealLocalizableRealPositionableWrapper<>(source)
@tpietzsch

This comment has been minimized.

Show comment
Hide comment
@tpietzsch

tpietzsch Feb 7, 2018

Member

@awalter17
I pushed some changes to branch shape-roi-fixes.

Please remove generic parameters from the remaining non-writable interfaces. I did it for Box here: 2773f4d

Alternatives to the name Writable would be Mutable or Modifiable. I'm fine with writable, but maybe somebody else has preferences? (@axtimwalde @ctrueden @maarzt @hanslovsky @StephanPreibisch)

Member

tpietzsch commented Feb 7, 2018

@awalter17
I pushed some changes to branch shape-roi-fixes.

Please remove generic parameters from the remaining non-writable interfaces. I did it for Box here: 2773f4d

Alternatives to the name Writable would be Mutable or Modifiable. I'm fine with writable, but maybe somebody else has preferences? (@axtimwalde @ctrueden @maarzt @hanslovsky @StephanPreibisch)

awalter17 added some commits Feb 7, 2018

Switch WritableRealPointCollection to use RealLocalizable
The elements in the collection only need to be RealLocalizable, not
RealPositionable.
@awalter17

This comment has been minimized.

Show comment
Hide comment
@awalter17

awalter17 Feb 7, 2018

Contributor

Thanks @tpietzsch! I added your commits from shape-roi-fixes to shape-rois and removed the generic parameters on the remaining non-writable interfaces except RealPointCollection.

Switching RealPointCollection to use just RealLocalizable felt a bit too constraining. One change I did make to WritableRealPointCollection was to just make the points in the collection have to be RealLocalizable not RealLocalizable & RealPositionable. Looking back, it just seemed weird to demand they be RealPositionable.

As for the naming (writable, etc.), I have no strong feelings. I am happy to change it to something else, if need be!

Contributor

awalter17 commented Feb 7, 2018

Thanks @tpietzsch! I added your commits from shape-roi-fixes to shape-rois and removed the generic parameters on the remaining non-writable interfaces except RealPointCollection.

Switching RealPointCollection to use just RealLocalizable felt a bit too constraining. One change I did make to WritableRealPointCollection was to just make the points in the collection have to be RealLocalizable not RealLocalizable & RealPositionable. Looking back, it just seemed weird to demand they be RealPositionable.

As for the naming (writable, etc.), I have no strong feelings. I am happy to change it to something else, if need be!

@ctrueden

This comment has been minimized.

Show comment
Hide comment
@ctrueden

ctrueden Feb 7, 2018

Member

Writable is OK by me also. The precedent is limited and mixed already:

Writable

fiji/Video_Editing/src/main/java/video2/WritableVirtualStack.java
imglib/imglib2-algorithm/src/main/java/net/imglib2/algorithm/gauss/WritableLineIterator.java
scifio/scifio-jai-imageio/src/main/java/io/scif/media/imageioimpl/plugins/gif/GIFWritableImageMetadata.java
scifio/scifio-jai-imageio/src/main/java/io/scif/media/imageioimpl/plugins/gif/GIFWritableStreamMetadata.java

Mutable

scijava/scijava-common/src/main/java/org/scijava/module/DefaultMutableModuleInfo.java
scijava/scijava-common/src/main/java/org/scijava/module/DefaultMutableModule.java
scijava/scijava-common/src/main/java/org/scijava/module/MutableModuleItem.java
scijava/scijava-common/src/main/java/org/scijava/module/DefaultMutableModuleItem.java
scijava/scijava-common/src/main/java/org/scijava/module/MutableModule.java
scijava/scijava-common/src/main/java/org/scijava/module/MutableModuleInfo.java
imagej/imagej-ops/src/main/java/net/imagej/ops/special/OutputMutable.java
Member

ctrueden commented Feb 7, 2018

Writable is OK by me also. The precedent is limited and mixed already:

Writable

fiji/Video_Editing/src/main/java/video2/WritableVirtualStack.java
imglib/imglib2-algorithm/src/main/java/net/imglib2/algorithm/gauss/WritableLineIterator.java
scifio/scifio-jai-imageio/src/main/java/io/scif/media/imageioimpl/plugins/gif/GIFWritableImageMetadata.java
scifio/scifio-jai-imageio/src/main/java/io/scif/media/imageioimpl/plugins/gif/GIFWritableStreamMetadata.java

Mutable

scijava/scijava-common/src/main/java/org/scijava/module/DefaultMutableModuleInfo.java
scijava/scijava-common/src/main/java/org/scijava/module/DefaultMutableModule.java
scijava/scijava-common/src/main/java/org/scijava/module/MutableModuleItem.java
scijava/scijava-common/src/main/java/org/scijava/module/DefaultMutableModuleItem.java
scijava/scijava-common/src/main/java/org/scijava/module/MutableModule.java
scijava/scijava-common/src/main/java/org/scijava/module/MutableModuleInfo.java
imagej/imagej-ops/src/main/java/net/imagej/ops/special/OutputMutable.java
@hanslovsky

This comment has been minimized.

Show comment
Hide comment
@hanslovsky

hanslovsky Feb 7, 2018

Member

I do not have a very strong opinion on this as I did not have time to go through the PR in detail (I read, and I think understood, most of the discussion, though), but I have a suggestion for a different identifier for mutable/writable shapes:
If I understand correctly, we do have an immutable implementation (e.g. Box) and one that we can move (by setting the center, e.g. WritableBox) for each ROI shape. If that is true, I think that Positionable as a prefix would be more expressive than Writable or Mutable, e.g. PositionableBox.

Member

hanslovsky commented Feb 7, 2018

I do not have a very strong opinion on this as I did not have time to go through the PR in detail (I read, and I think understood, most of the discussion, though), but I have a suggestion for a different identifier for mutable/writable shapes:
If I understand correctly, we do have an immutable implementation (e.g. Box) and one that we can move (by setting the center, e.g. WritableBox) for each ROI shape. If that is true, I think that Positionable as a prefix would be more expressive than Writable or Mutable, e.g. PositionableBox.

@awalter17

This comment has been minimized.

Show comment
Hide comment
@awalter17

awalter17 Feb 8, 2018

Contributor

If I understand correctly, we do have an immutable implementation (e.g. Box) and one that we can move (by setting the center, e.g. WritableBox) for each ROI shape. If that is true, I think that Positionable as a prefix would be more expressive than Writable or Mutable, e.g. PositionableBox.

@hanslovsky It is true that WritableBox is positionable by setting the center. However, it also allows the user to mutate the side lengths. So, I think Positionable would be too specific of a prefix.

Contributor

awalter17 commented Feb 8, 2018

If I understand correctly, we do have an immutable implementation (e.g. Box) and one that we can move (by setting the center, e.g. WritableBox) for each ROI shape. If that is true, I think that Positionable as a prefix would be more expressive than Writable or Mutable, e.g. PositionableBox.

@hanslovsky It is true that WritableBox is positionable by setting the center. However, it also allows the user to mutate the side lengths. So, I think Positionable would be too specific of a prefix.

@ctrueden

This comment has been minimized.

Show comment
Hide comment
@ctrueden

ctrueden Feb 8, 2018

Member

I think Positionable would be too specific of a prefix.

I second that—the PositionableBox would not actually implement Positionable so I think that name would be misleading. (ExtendedRandomAccessibleInterval I'm looking at you! 👁)

Member

ctrueden commented Feb 8, 2018

I think Positionable would be too specific of a prefix.

I second that—the PositionableBox would not actually implement Positionable so I think that name would be misleading. (ExtendedRandomAccessibleInterval I'm looking at you! 👁)

@hanslovsky

This comment has been minimized.

Show comment
Hide comment
@hanslovsky

hanslovsky Feb 8, 2018

Member

Thank you for the clarification @awalter17 and @ctrueden
I am fine with picking any of the suggested prefixes, then.

Member

hanslovsky commented Feb 8, 2018

Thank you for the clarification @awalter17 and @ctrueden
I am fine with picking any of the suggested prefixes, then.

@imagejan

This comment has been minimized.

Show comment
Hide comment
@imagejan

imagejan Feb 13, 2018

Contributor

I'd slightly prefer Mutable as it is in line with MutableModule etc. in SciJava, and it's shorter by one letter :-)

Otherwise, is this PR now ready to be merged?

Contributor

imagejan commented Feb 13, 2018

I'd slightly prefer Mutable as it is in line with MutableModule etc. in SciJava, and it's shorter by one letter :-)

Otherwise, is this PR now ready to be merged?

@tpietzsch tpietzsch merged commit a2e4725 into master Feb 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tpietzsch

This comment has been minimized.

Show comment
Hide comment
@tpietzsch

tpietzsch Feb 13, 2018

Member

yep, it's finally merged... Thanks @awalter17 and everyone who contributed/reviewed

Member

tpietzsch commented Feb 13, 2018

yep, it's finally merged... Thanks @awalter17 and everyone who contributed/reviewed

@tpietzsch tpietzsch deleted the shape-rois branch Feb 13, 2018

@tpietzsch

This comment has been minimized.

Show comment
Hide comment
@tpietzsch

tpietzsch Feb 13, 2018

Member

... and released (imglib2-roi 0.5.0)

Member

tpietzsch commented Feb 13, 2018

... and released (imglib2-roi 0.5.0)

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