Comments (16)
To explain a little: it is weird that ImgFactory
takes a type T
on the class, but you do not pass anything which anchors that to the constructor invocation. So any ImgFactory
created via reflection at runtime, for example, does not even know its own type.
One question is: why does an ImgFactory
need a type at all? If the type is simply passed to the create
method calls, that should be sufficient, right? And actually it already is. If the type param were removed from the class itself, we wouldn't need the imgFactory
method anymore for switching types.
Alternately, option 1 binds each ImgFactory
instance to a specific type T
, but then eliminates the need to pass an explicit type instance every time create
is called. This avoids the chance of runtime "skew" between the intended type of the ImgFactory
, and the actual type of the type instance passed to create
.
Both options above are proposed so that backwards compatibility is maintainedโi.e., so we don't have to bump to imglib2-3.0.0
.
from imglib2.
@tpietzsch Any chance you are interested in a PR addressing this issue? I can file one, but would need to know which option above you feel is more appropriate. And of course, depending on the choice, it could break backwards compatibility, so might want to wait a while till we are ready for imglib2 5.0.0.
from imglib2.
@ctrueden Yes, I'm interested. I also find the need to specify the type twice annoying. In principle I would prefer option 2: removing the type from the factory. But I'm not sure whether that is possible because NativeImgFactory
could not extend ImgFactory
anymore. But I'm not sure and it might be worth trying.
@axtimwalde, can you comment on this please?
from imglib2.
@tpietzsch @ctrueden without much though I am all for it. The type belongs into the factory methods not into the factory. However, a type in the factory could provide bounds to the type methods are willing to accept which would enable to have NativeImgFactory extends ImgFactory
. I tried that quickly and it basically breaks all of ImgLib2 and will not go easy without significant API overhaul. Better ideas?
from imglib2.
Doing anything like NativeImgFactory
vs ImgFactory
requires a type parameter on the factory, otherwise we cannot narrow applicability to only specific types. It's certainly a bigger project, but if we can find a nice solution it would be worth to break API. I have thought about it for a while and I do not see a perfect solution so far.
Related (but possibly orthogonal) things that we should do while we are at it:
1.) Make ImgFactory
an interface.
2.) Split the part of NativeImgFactory
that is consumed by NativeType.createSuitableNativeImg
into a separate interface. There is no reason to mash these into the same inheritance hierarchy. (This way, if the user gets a NativeImgFactory
(by interface) he does not see the strange createLongInstance
, createFloatInstance
, etc methods that make absolutely no sense in particular if the factory is typed.)
In principle there could be just a untyped ImgFactory
interface with a createGeneric
or tryCreate
method that throws UnsupportedTypeException
. More specific factories could add more concrete create
methods that have bounds on the method parameters. I already have such a create method in ReadOnlyCachedCellImgFactory
(which does not extend ImgFactory which allows me to do this...)
from imglib2.
How about this? https://github.com/imglib/imglib2/compare/img-factory
It implements Option 1 above, which is much less destabilizing to the rest of the ImgLib2 codebase and downstream components. It was slightly more complex than stated above due to the introduction of the Supplier< T >
option since this issue was written, but actually it's great because now the supplier is handled uniformly across all ImgFactory
implementations.
This change also improves the type safety and elegance of the imgFactory( S type )
methods.
If you guys like it, I can file it as a PR.
@tpietzsch Regarding your recent comments above: I like what you are saying, but as you point out, those changes would be breaking. Should we merge my work here, and then leave this issue open as a reminder that we ultimately want to purge the generic property from ImgFactory
and make the other changes you describe?
from imglib2.
@ctrueden Hmm, it is better than nothing, but is doesn't solve the problem in my opinion. I would rather try to come up with a better solution for a little bit longer, instead of making this a PR now and breaking API twice.
At the moment at least we are able to do
final ImgFactory factory = new ArrayImgFactory();
final Img< ByteType > bytes = factory.create( new int[] { 10, 10, 10 }, new ByteType() );
final Img< ShortType > shorts = factory.create( new int[] { 10, 10, 10 }, new ShortType() );
if we are not being nice and using raw types.
I think that, this is what we would actually want (but if possible more type-safe). We want to reuse the same factory to instead of creating a new factory for every new image. Ideally ArrayImgFactory
would be a singleton.
from imglib2.
@ctrueden I thought more about it: Your branch would break existing code. All existing code creates ArrayImgFactory
etc without a "factory type" parameter. So all create
calls will throw NPE. This could be fixed by redirecting the new create
-without-type calls to the deprecated ones, instead of the other way around. Even then, it is only safe to use the new create
-without-type once we made sure factories everywhere are constructed with the constructor-with-type. There is no gradual migration path. Let's face it: this requires a breaking change.
from imglib2.
So all create calls will throw NPE.
No, only the new ones that do not pass the type. For existing code, everything should still work. The only thing that would break are third party classes extending ImgFactory
and friends, I think.
from imglib2.
No, only the new ones that do not pass the type. For existing code, everything should still work. The only thing that would break are third party classes extending ImgFactory and friends, I think.
True, I saw that wrong.
Still the problem remains that we cannot use the create
-without-type methods anywhere inside an algorithm for example because the factory passed in might be constructed with the old constructor-without-type.
In my opinion this is a mess and I would rather break API and fix code everywhere to set it right.
from imglib2.
@ctrueden @axtimwalde I also yesterday made several futile attempts to come up with an alternative solution. I think, @ctrueden solution is the only way that makes sense.
ImgFactory
has to remain typed, otherwise we cannot restrict to NativeType
in derived factories.
That means there has to be new factory instances for every new type we want to create.
These we get by
public abstract < S > ImgFactory< S > imgFactory( final S type ) throws IncompatibleTypeException;
It's infuriating that we cannot even derive this to return something more specific that ImgFactory
, such as CellImgFactory<S>
in CellImgFactory
. But I guess everyone gets the type system they deserve ... :-)
In the end, instead of doing
factory.create( IntType::new, 10, 10, 10 );
it will be
factory.imgFactory( IntType::new ).create( 10, 10, 10 );
which is almost as convenient.
We might consider renaming imgFactory
to forType
factory.forType( IntType::new ).create( 10, 10, 10 );
Still the problem remains that we cannot use the create-without-type methods anywhere inside an algorithm for example because the factory passed in might be constructed with the old constructor-without-type.
This could be fixed (almost) if the concrete factories would make new typed factory instances for the image instances they create.
public ArrayImg< T, ? > create( final long[] dim, final T type )
{
return ( ArrayImg< T, ? > ) type.createSuitableNativeImg( this, dim );
}
would need to become
public ArrayImg< T, ? > create( final long[] dim, final T type )
{
return ( ArrayImg< T, ? > ) type.createSuitableNativeImg( new ArrayImgFactory<>(type), dim );
}
etc.
And we need to remember to revert this once we remove the deprecated methods.
With this change I would be fine with making this into a PR, (...being finally convinced that this sets up the only sensible API ๐ข )
@ctrueden If you are not annoyed too much by this already, please go ahead...
On a related topic: I would like to make IncompatibleTypeException
into an unchecked exception (like IllegalArgumentException
), because there are many cases where you just know that the exception cannot be thrown in the
factory.forType( IntType::new ).create( 10, 10, 10 );
and it is cumbersome to always write the empty try-catch boilerplate there.
Coincidentally, @maarzt came today with the same suggestion, but on a different problem. He will make an issue for this.
from imglib2.
@tpietzsch wrote:
This could be fixed (almost) if the concrete factories would make new typed factory instances for the image instances they create.
What if, instead of the ImgFactory
having a final T t
field, we removed the final
modifier, and then if the t
is still null, cache it when it is passed to one of the deprecated create
methods? Then we could keep using this
for createSuitableNativeImg
.
In that way, any "bad" (no available type instance) ImgFactory
becomes a "good" one as soon as any of the API methods are called that pass the instance.
from imglib2.
sounds good
from imglib2.
OK, working on it now!
from imglib2.
๐ thanks
from imglib2.
Resolved by #197. Thanks @tpietzsch!
from imglib2.
Related Issues (20)
- Building a kdTree is slow for repeated values HOT 1
- More specific type hierarchy for volatile types
- Remove unnecessary overrides of IterableRealInterval.iterator()
- Spliterators for standard NativeImgs
- Create LocalizableSamplerStream HOT 4
- Explore consequences of adding ImgLibStream wrapper HOT 1
- Stream API documentation
- Improve Point.setPosition
- Consistent toString() implementations
- Deprecate AbstractLocalizingCursor
- Simplify AbstractArrayCursor hierarchy
- Reduce allocation size for Cell objects in CellImg
- Add Util.printCoordinates(Localizable)
- Implement CellLocalizingSpliterator and PlanarLocalizingSpliterator
- Simplify static creation methods
- Views.bundle returns BundleView, but BundleView class is not public HOT 3
- Modularize core ImgLib2 projects HOT 1
- Improve Intervals methods
- WriteConvertedCursor constructor fails with ArrayIndexOutOfBoundsException HOT 6
- Add documentation for SamplerConverter
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. ๐๐๐
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google โค๏ธ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from imglib2.