Add RandomAccessibleInterval.getType #312

hanslovsky · 2022-02-25T15:14:38Z

Adding getType to the RandomAccessibleInterval interface would allow individual implementations to provide more efficient implementations. For example, loading the voxel at the min of the RAI may be expensive for some implementations of NativeImg. Instead, those implementations can use their linkedType to create a copy of the associated type, if available.

Discussion on gitter for reference

Open questions:

We cannot deprecate Utils.getTypeFromInterval because it does not use RAI as argument. We could change the argument to RAI but this would be a breaking change (see comment in JavaDoc for that function)
What about getType in other places like RA, RRA, RRARI?
Name preference getType vs type?

acardona · 2022-02-25T15:19:48Z

Thanks for this @hanslovsky! Been bitten by this many times when using lazy cell loading: entire large blocks are loaded (e.g., when a Cell is the whole 2D section, representing a whole 1 GB file). Would have been nice to have a way to return the known type without having to load 1 GB files that won't be used otherwise.

gselzer · 2022-02-25T15:28:42Z

We cannot deprecate Utils.getTypeFromInterval because it does not use RAI as argument. We could change the argument to RAI but this would be a breaking change (see comment in JavaDoc for that function)

Is there a reason not to move this method into RandomAccessible? You could still have the default implementation in RAI, and then you could deprecate getTypeFromInterval.

What about getType in other places like RA, RRA, RRARI?

If you moved this method to RA, then you'd get it in all of these places.

Name preference getType vs type?

I like type, it is shorter and aligns with other methods you might call on a RAI (like min, max, dimension, randomAccess, etc.)

gselzer · 2022-02-25T15:33:47Z

Is there a reason not to move this method into RandomAccessible?

Ah, you'd want a default implementation in RandomAccessible if you added it, that is a good reason. You could hack it with Views.interval(this, <arbitrary interval>).getType() 😀

hanslovsky · 2022-02-28T03:37:17Z

Two additional commits:

7be3635 adds getType() to interfaces RA, RRA, RRARI
1f67b63 adds more efficient implementation of getType() to some classes

hanslovsky · 2022-02-28T03:38:24Z

@gselzer

Ah, you'd want a default implementation in RandomAccessible if you added it, that is a good reason. You could hack it with Views.interval(this, <arbitrary interval>).getType()

Yes, that is correct. You can see how I solved this in 7be3635

gselzer · 2022-02-28T04:19:51Z

@gselzer

Ah, you'd want a default implementation in RandomAccessible if you added it, that is a good reason. You could hack it with Views.interval(this, <arbitrary interval>).getType()

Yes, that is correct. You can see how I solved this in 7be3635

Aha, randomAccess().getType() is a better solution. Looks great @hanslovsky!

hanslovsky · 2022-03-02T02:58:54Z

Adding more implementations for getType(), I realized that using a default implementation may not be very useful. This already breaks if a wrapping RAI does not override it, e.g. something like this is possibly slow if the converted RAI does not implement a good way to provide getType():

RAI<T> raiWithEfficientGetType = ...
raiWithEfficientGetType.getType(); // this is fast
RAI<U> notEfficient = Converters.convert(raiWithEfficientGetType, ...);
notEfficient.getType(); // this is slow

Several options:

make it not default. Probably a big no. All downstream libraries will have to implement this, unless it is covered by some abstract classes in imglib2 core
implement it for all concrete and abstract classes in imglib2 core. This should cover most of what people use regularly.
discard the changes. This would be sad, too.
Might need to go with option (2).

hanslovsky · 2022-03-02T04:30:46Z

In dc0b039, I added an example of how an implementation for (most) of RA/RAI could look like. This just for demonstration purposes and can be easily reverted. I did not override getType for these classes:

net.imglib2.img.cell.LazyCellImg.LazyCells
net.imglib2.img.cell.CellGrid.CellIntervals
net.imglib2.img.list.ListImg
net.imglib2.util.Localizables.LocationRandomAccessible
net.imglib2.view.FunctionView
net.imglib2.view.composite.CompositeView
net.imglib2.view.composite.CompositeIntervalView
net.imglib2.converter.readwrite.WriteConvertedIterableRandomAccessibleInterval
net.imglib2.view.HyperSlicesView
net.imglib2.converter.read.ConvertedRandomAccessible
net.imglib2.img.cell.LazyCellImg.LazyCells
net.imglib2.img.cell.CellGrid.CellIntervals
net.imglib2.img.list.ListImg
net.imglib2.util.Localizables.LocationRandomAccessible
net.imglib2.view.FunctionView
net.imglib2.view.composite.CompositeView
net.imglib2.view.composite.CompositeIntervalView
net.imglib2.converter.readwrite.WriteConvertedIterableRandomAccessibleInterval
net.imglib2.view.HyperSlicesView
net.imglib2.converter.read.ConvertedRandomAccessible

gselzer · 2022-03-02T14:38:18Z

Adding more implementations for getType(), I realized that using a default implementation may not be very useful. This already breaks if a wrapping RAI does not override it, e.g. something like this is possibly slow if the converted RAI does not implement a good way to provide getType():

What do you mean here, @hanslovsky? How does it break?

hanslovsky · 2022-03-03T01:33:54Z

@gselzer Let me provide a different example: A use case that probably is not that uncommon is to take an smaller interval from a larger RAI.
Let's assume that the large RAI is a cached cell img that is backed by a slow loader, e.g. fetching data over the internet.

RAI<T> cachedImg = ...

the cached cell img happens to override getType() efficiently in this example, so

cachedImg.getType()

is a very fast operation. The user now goes ahead and takes a smaller interval out of this cached cell img, i.e.

RAI<T> interval = Views.interval(cachedImg, ...);

The user also knows that cachedImg.getType() is very efficient and expects the same for interval. In this example, the class returned by Views.interval does not override getType() and falls back to the default implementaiton, i.e. go through the random access and actually load the data from the cached cell img that backs this smaller interval. Hence,

interval.getType()

is very slow, and the user has no way of making it fast, again.

This is, of course a hypothetical example, but might easily happen if we provide a default implementation for getType().

gselzer · 2022-03-03T01:59:09Z

Huh, in the case of Views, can they not just delegate to the getType() of the source RA?

tischi · 2022-03-03T07:01:33Z

Huh, in the case of Views, can they not just delegate to the getType() of the source RA?

But I guess all of this means having to touch quite some code in imglib2, I think something that @axtimwalde was afraid of when discussing this in gitter?! Personally, as my "cloud hosted image" use-cases (and I guess more people will have such use-cases in the future) are suffering quite a bit from this issue I would still be in favor of trying to find a solution.

@hanslovsky for the Converters, don't you actually have to provide a type when using them?

For example:

final static public < A, B extends Type< B > > RandomAccessible< B > convert(
			final RandomAccessible< A > source,
			final Converter< ? super A, ? super B > converter,
			final B b ) // <= here
	{
		if ( TypeIdentity.class.isInstance( converter ) )
			return ( RandomAccessible< B > ) source;
		return new ConvertedRandomAccessible<>( source, converter, b );
	}

And then ConvertedRandomAccessible has a method, which feels close to the getType() that we would like to have:

public B getDestinationType()
{
	return converted.copy();
}

Does that help?

EDIT: Sorry, I think my comments are in fact obsolete as they are already covered here: dc0b039

hanslovsky · 2022-03-03T15:06:57Z

@gselzer

Huh, in the case of Views, can they not just delegate to the getType() of the source RA?

Yes, that is exactly my point. They will have to delegate to make it any useful. That's why I don't like the default implementation because it will hide all those cases.

gselzer · 2022-03-03T15:13:38Z

Yes, that is exactly my point. They will have to delegate to make it any useful. That's why I don't like the default implementation because it will hide all those cases.

Well, what do you mean by 'make it any useful'? I think that your default RA implementation is useful, because that is what someone would do to get its type if they did not know anything more about what the implementation was. Sure, it is slow, but do you have any alternative when you have a RA?

hanslovsky · 2022-03-03T15:31:17Z

It will not provide any benefit over what is already there unless we implement it for all classes.

gselzer · 2022-03-03T15:40:58Z

It will not provide any benefit over what is already there unless we implement it for all classes.

Well, it might if a given implementation has implemented it, no? If you have a RA of unknown implementation, you have a chance of improved performance with getType(), as opposed to it always being slow otherwise.

Anyways, I'll delegate to your judgement here, the PR is a fantastic addition either way 😁

hanslovsky · 2022-03-03T15:48:30Z

Well, it might if a given implementation has implemented it, no? If you have a RA of unknown implementation, you have a chance of improved performance with getType(), as opposed to it always being slow otherwise.

Generally, I agree, as long as a simple operation like Views.interval or Views.zeroMin etc does not hide the performance improvement. This should be mitigated (to some extent) by the added implementations within imglib2 core in this PR. This will likely cover a lot of the use cases already. There may be some additional implementations, e.g. in imglib2-realtransform that should override the default implementation.

Anyways, I'll delegate to your judgement here, the PR is a fantastic addition either way 😁

Thank you, I appreciate it. I will leave it to the maintainers (@tpietzsch @axtimwalde @StephanPreibisch) to decide what part of the PR should go into the repo. My recommendation is to add implementations wherever possible for classes that implement RA or RAI. I will also add similar implementations for classes that implement RRA or RRARI.

hanslovsky · 2022-03-03T20:56:49Z

I added implementations for RRA and RRARI classes as well. The only remaining classes without explicit implementaiton in imglib2 core are:

net.imglib2.img.cell.LazyCellImg.LazyCells
net.imglib2.img.cell.CellGrid.CellIntervals
net.imglib2.img.list.ListImg
net.imglib2.util.Localizables.LocationRandomAccessible
net.imglib2.view.FunctionView
net.imglib2.view.composite.CompositeView
net.imglib2.view.composite.CompositeIntervalView
net.imglib2.interpolation.Interpolant
net.imglib2.converter.readwrite.WriteConvertedIterableRandomAccessibleInterval
net.imglib2.view.HyperSlicesView
net.imglib2.img.cell.LazyCellImg.LazyCells
net.imglib2.img.cell.CellGrid.CellIntervals
net.imglib2.img.list.ListImg
net.imglib2.util.Localizables.LocationRandomAccessible
net.imglib2.view.FunctionView
net.imglib2.view.composite.CompositeView
net.imglib2.view.composite.CompositeIntervalView
net.imglib2.interpolation.Interpolant
net.imglib2.converter.readwrite.WriteConvertedIterableRandomAccessibleInterval
net.imglib2.view.HyperSlicesView

hanslovsky · 2023-10-04T13:33:06Z

I recently ran into this again, where a more efficient way to retrieve the type would have been very helpful. I rebased over current master/main and marked as ready for review.

hanslovsky · 2023-10-04T16:39:20Z

I created this little kscript to demonstrate the behavior + fix:

@file:Repository("https://maven.scijava.org/content/groups/public")
// @file:Repository("https://jitpack.io") // uncomment for getType fix
// @file:DependsOn("com.github.imglib:imglib2:feature-rai-getType-SNAPSHOT") // uncomment for getType fix
@file:DependsOn("net.imglib2:imglib2:6.2.0") // uncomment for current behavior
@file:DependsOn("net.imglib2:imglib2-cache:1.0.0-beta-17")

import net.imglib2.cache.img.CachedCellImg
import net.imglib2.cache.img.CellLoader
import net.imglib2.cache.ref.SoftRefLoaderCache
import net.imglib2.img.basictypeaccess.volatiles.array.VolatileIntArray
import net.imglib2.img.cell.Cell
import net.imglib2.img.cell.CellGrid
import net.imglib2.type.numeric.integer.IntType
import net.imglib2.util.Util

val dims = longArrayOf(10L)
val chunks = intArrayOf(1)
val grid = CellGrid(dims, chunks)

val cache = SoftRefLoaderCache<Long, Cell<VolatileIntArray>>().withLoader {
    println("Loading cell $it")
    Cell(chunks, longArrayOf(it), VolatileIntArray(1, true))
}

val img = CachedCellImg(grid, IntType(), cache, VolatileIntArray(1, true))

cache.invalidateAll(5000)
println("getTypeFromInterval")
Util.getTypeFromInterval(img)
cache.invalidateAll(5000)
println("getLinkedType")
img.createLinkedType()
println("getTypeFromInterval")
Util.getTypeFromInterval(img)

Output with imglib2-6.2.0:

getTypeFromInterval
Loading cell 0
getLinkedType
getTypeFromInterval
Loading cell 0

Output with this PR:

getTypeFromInterval
getLinkedType
getTypeFromInterval

hanslovsky · 2023-10-06T18:25:18Z

src/main/java/net/imglib2/converter/readwrite/WriteConvertedRandomAccessibleInterval.java

+		return converterSupplier.get().convert( ( Sampler< ? extends A > ) new ConstantSampler( getSource().getType() ) );
+	}
+
+	private static class ConstantSampler< T > implements Sampler< T >


Is duplicated, could be extracted and shared between WriteConvertedRandomAccessibleInterval and WriteConvertedRandomAccessible.

hanslovsky · 2023-10-06T18:28:36Z

src/main/java/net/imglib2/img/AbstractNativeImg.java

+	@Override
+	public T getType() {
+		try {
+			return linkedType.createVariable();
+		} catch ( final NullPointerException e ) {
+			return super.getType();
+		}
+	}


This could move into the NativeImage interface as

@Override public default T getType() { return createLinkedType().createVariable(); }

Is there a performance penalty to creating a linked type vs just createVariable()? Is this null safe or should we add a null check ?

hanslovsky · 2023-10-06T18:30:37Z

src/main/java/net/imglib2/util/Util.java

+		// TODO can we remove generic parameter F and use
+		//      RandomAccessible<T> rai instead?
+		//      This would be a breaking change, though.


I suggest we just leave the generic parameter as is.

hanslovsky · 2023-10-06T18:40:12Z

src/test/java/net/imglib2/RandomAccessibleIntervalTest.java

+		// setup
+		final RandomAccessibleInterval< ? > rai = ArrayImgs.bytes( 1 );
+		// process & test
+		assertEquals( ByteType.class, rai.getType().getClass() );


This test is not very useful. Should we remove it?

I like it! A simple sanity check.

tpietzsch · 2024-04-05T20:26:13Z

@ctrueden I looked at all the errors and made draft PRs for every failing project (always branch feature-rai-getType)
The only problem I ran into is I think unrelated: juglab/labkit-ui#115 (comment)
Apart from that there should be no more problems.

Open questions: - We cannot deprecate Utils.getTypeFromInterval because it does not use RAI as argument. We could change the argument to RAI but this would be a breaking change (see comment in JavaDoc for that function) - What about getType in other places like RA, RRA, RRARI? - Name preference getType vs type?

In many cases, it may be more efficient to delegate getType to a wrapped source

IterableRealInterval and RandomAccessible cannot both have default getType() implementations, otherwise we run into a bug with generic bounds: https://bugs.openjdk.org/browse/JDK-7120669 Therefore we implement getType for all IterableRealInterval implementations. For the same reason, the getType() method must be inherited from a common super-type. Possibly it is possible to avoid this by not having a default implementation at all. Something to try. On the other hand, a Typed<T> interface is probably a good idea.

Instead, implement getType() in all RandomAccessible[Interval] implementations

Implement getType() in all Sampler implementations

Instead implement it in all Real...Accessible implementation

See #312 (comment)

…if no dataAccess is set

See #312 (comment)

tischi · 2024-05-07T11:29:38Z

WOW 🥳

Thanks for all the work!

imagesc-bot · 2024-05-08T21:55:29Z

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/recent-and-upcoming-imglib2-improvements/96083/2

hanslovsky force-pushed the feature-rai-getType branch 2 times, most recently from 49ef92d to f27e986 Compare February 25, 2022 15:17

bogovicj mentioned this pull request Feb 25, 2022

get type from typed interfaces #311

Closed

tischi mentioned this pull request Apr 23, 2022

Optimize loading from S3 mobie/mobie-viewer-fiji#677

Closed

hanslovsky force-pushed the feature-rai-getType branch from dc0b039 to dc07263 Compare October 4, 2023 13:06

hanslovsky marked this pull request as ready for review October 4, 2023 13:31

hanslovsky commented Oct 6, 2023

View reviewed changes

This was referenced Apr 5, 2024

Addition of getType() API saalfeldlab/bigwarp#170

Draft

Addition of getType() API juglab/labkit-pixel-classification#12

Draft

Addition of getType() API juglab/labkit-ui#115

Draft

Addition of getType() API saalfeldlab/z-spacing#28

Draft

hanslovsky and others added 18 commits April 16, 2024 17:23

Add getType to RA, RRA, RRARI

12dce9c

Override getType for efficiency in some classes

d1baade

In many cases, it may be more efficient to delegate getType to a wrapped source

Add getType implementations for most of RA/RAI classes.

dc135ca

formatting and javadoc changes

c8517ac

Remove default RandomAccessible[Interval]::getType implementations

452f0b7

Instead, implement getType() in all RandomAccessible[Interval] implementations

Let Sampler<T> extend Typed<T>

2c14cf1

Implement getType() in all Sampler implementations

Remove default getType in Real..Accessibles

6205461

Instead implement it in all Real...Accessible implementation

clean up

14a5b10

formatting

9d61a64

Replace usage of Util.getTypeFromInterval(rai) with rai.getType()

c60ce48

Add default getType impl for Sampler and IterableRealInterval

857d48f

javadoc fixes

e4f0eef

Util.getTypeFromInterval returns getType().createVariable() is possible

c1b4576

See #312 (comment)

Replace calls of Type.copy() that should actually be createVariable()

96c8315

Fix circular call to LanczosInterpolator.copy()

05e77c1

POM: Bump version to 7.0.0-SNAPSHOT

d10e744

tpietzsch force-pushed the feature-rai-getType branch from 359f407 to d10e744 Compare April 16, 2024 15:33

Be defensive in NativeType.copy() methods: Act like createVariable() …

60aff89

…if no dataAccess is set

tpietzsch merged commit 4b97c08 into master May 7, 2024
1 check passed

tpietzsch added a commit that referenced this pull request May 7, 2024

Util.getTypeFromInterval returns getType().createVariable() is possible

add5f6d

See #312 (comment)

tpietzsch deleted the feature-rai-getType branch May 7, 2024 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RandomAccessibleInterval.getType #312

Add RandomAccessibleInterval.getType #312

hanslovsky commented Feb 25, 2022

acardona commented Feb 25, 2022

gselzer commented Feb 25, 2022

gselzer commented Feb 25, 2022 •

edited

Loading

hanslovsky commented Feb 28, 2022

hanslovsky commented Feb 28, 2022

gselzer commented Feb 28, 2022

hanslovsky commented Mar 2, 2022

hanslovsky commented Mar 2, 2022

gselzer commented Mar 2, 2022

hanslovsky commented Mar 3, 2022

gselzer commented Mar 3, 2022

tischi commented Mar 3, 2022 •

edited

Loading

hanslovsky commented Mar 3, 2022

gselzer commented Mar 3, 2022

hanslovsky commented Mar 3, 2022

gselzer commented Mar 3, 2022

hanslovsky commented Mar 3, 2022

hanslovsky commented Mar 3, 2022

hanslovsky commented Oct 4, 2023

hanslovsky commented Oct 4, 2023

hanslovsky Oct 6, 2023

hanslovsky Oct 6, 2023 •

edited

Loading

hanslovsky Oct 6, 2023

hanslovsky Oct 6, 2023

ctrueden Apr 2, 2024

tpietzsch commented Apr 5, 2024

tischi commented May 7, 2024

imagesc-bot commented May 8, 2024

Add RandomAccessibleInterval.getType #312

Add RandomAccessibleInterval.getType #312

Conversation

hanslovsky commented Feb 25, 2022

acardona commented Feb 25, 2022

gselzer commented Feb 25, 2022

gselzer commented Feb 25, 2022 • edited Loading

hanslovsky commented Feb 28, 2022

hanslovsky commented Feb 28, 2022

gselzer commented Feb 28, 2022

hanslovsky commented Mar 2, 2022

hanslovsky commented Mar 2, 2022

gselzer commented Mar 2, 2022

hanslovsky commented Mar 3, 2022

gselzer commented Mar 3, 2022

tischi commented Mar 3, 2022 • edited Loading

hanslovsky commented Mar 3, 2022

gselzer commented Mar 3, 2022

hanslovsky commented Mar 3, 2022

gselzer commented Mar 3, 2022

hanslovsky commented Mar 3, 2022

hanslovsky commented Mar 3, 2022

hanslovsky commented Oct 4, 2023

hanslovsky commented Oct 4, 2023

hanslovsky Oct 6, 2023

Choose a reason for hiding this comment

hanslovsky Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

hanslovsky Oct 6, 2023

Choose a reason for hiding this comment

hanslovsky Oct 6, 2023

Choose a reason for hiding this comment

ctrueden Apr 2, 2024

Choose a reason for hiding this comment

tpietzsch commented Apr 5, 2024

tischi commented May 7, 2024

imagesc-bot commented May 8, 2024

gselzer commented Feb 25, 2022 •

edited

Loading

tischi commented Mar 3, 2022 •

edited

Loading

hanslovsky Oct 6, 2023 •

edited

Loading