-
Notifications
You must be signed in to change notification settings - Fork 95
RealRandomAccessible: extend RandomAccessible #378
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
base: master
Are you sure you want to change the base?
Conversation
| * @return a {@link RandomAccessibleOnRealRandomAccessible} wrapping source. | ||
| * @return source | ||
| */ | ||
| public static < T > RandomAccessibleOnRealRandomAccessible< T > raster( final RealRandomAccessible< T > source ) | ||
| public static < T > RandomAccessible< T > raster( final RealRandomAccessible< T > source ) | ||
| { | ||
| return new RandomAccessibleOnRealRandomAccessible<>( source ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could consider removing this method entirely, as the return type change already warrants a major version bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still want this method, but I would be happy with the return type change. We could make this a separate PR though, and also break more API while we are at it. For example, I think it would be better if the Views methods would all return interface types instead of concrete classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emphatic yes to returning interface types!
We could make this a separate PR though, and also break more API while we are at it.
Are you suggesting leaving the return type as is for this PR then?
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/fiji-friends-weekly-dev-update-thread/103718/99 |
|
Ha! After thinking about it a bit, this makes sense, yes. A It wouldn't extend to I'm not sure about unintended side effects. For example I'm a bit reluctant. Could you give more context on why you need this? (I know it's somehow related to imglib2-meta, but I still haven't had time to dig into that. |
To be honest I hadn't fully thought that situation through, but yes, since
I guess I don't see the problem, at least as is... Where it gets trickier in that case is if we later want some BiConsumer<Localizable, DoubleType> c = (pos, out) -> out.setZero();
RealRandomAccessible<DoubleType> rra = new FunctionRealRandomAccessible<>(2, c, () -> new DoubleType());
RealRandomAccessibleRealInterval<DoubleType> rrari = Views.interval(rra, new FinalInterval(10, 10));you'll get method clashing, which is unfortunate. Of course, we could just implement this interval solution on the fluent Views API side and avoid the problem...
Yeah, my motivations with this PR come from imglib2-meta. One of the main goals of that project is to create a simple user-facing class Ideally, the Does that provide any helpful context? |
bc73d61 to
5011f13
Compare
It seems to me like the
RandomAccessibleAPI makes perfect sense when applied to aRealRandomAccessible.For that reason, shouldn't a
RealRandomAccessiblealso be aRandomAccessible?