Skip to content
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

CellRandomAccess unexpectedly loads neighboring cells #252

Open
igorpisarev opened this issue May 6, 2019 · 2 comments
Open

CellRandomAccess unexpectedly loads neighboring cells #252

igorpisarev opened this issue May 6, 2019 · 2 comments

Comments

@igorpisarev
Copy link

Let's say a CellImg is defined with cell size [2,2,2], and we want to iterate from min=[4,4,4] to max=[5,5,5] in this image. This interval covers exactly one cell at grid position [2,2,2].
However, looping over this interval using

Views.iterable(Views.interval(cellImg, cell)).cursor()

unexpectedly leads to loading 3 extra cells:

  • the very first one at (0,0,0,..) when the random access is initialized
  • the previous cell in the iteration order when the cursor is initialized at the "-1" pixel
  • the next cell in the iteration order when the cursor reaches the end of the line

This behavior becomes problematic in a workflow when the cells are read and written concurrently as it subtly introduces race conditions.

I also created an example demonstrating this behavior using N5 as a cell storage backend: https://gist.github.com/igorpisarev/591212a26228e4ac7cb482b0134217a1

@hanslovsky
Copy link
Member

Thanks for raising this issue @igorpisarev. I talked to @axtimwalde earlier and he said that he was having a look at the CellRandomAccess. Essentially, a cell should only be loaded as necessary on CellRandomAccess.get() and not on any CellRandomAccess.setPosition() or related methods.

@tischi
Copy link

tischi commented Jan 9, 2021

@axtimwalde @tpietzsch

I think I found another place where this hits one quite badly. It is in the StackView class:

		public DefaultRA( final RandomAccessibleInterval< T >[] slices, final Interval interval )
		{
			n = slices[ 0 ].numDimensions() + 1;
			sd = n - 1;
			slice = 0;
			tmpLong = new long[ sd ];
			tmpInt = new int[ sd ];
			sliceAccesses = new RandomAccess[ slices.length ];
			if ( interval == null )
			{
				for ( int i = 0; i < slices.length; ++i )
					sliceAccesses[ i ] = slices[ i ].randomAccess();
			}
			else
			{
				final long[] smin = new long[ sd ];
				final long[] smax = new long[ sd ];
				for ( int d = 0; d < sd; ++d )
				{
					smin[ d ] = interval.min( d );
					smax[ d ] = interval.max( d );
				}
				final Interval sliceInterval = new FinalInterval( smin, smax );
				for ( int i = 0; i < slices.length; ++i )
					sliceAccesses[ i ] = slices[ i ].randomAccess( sliceInterval );
			}
			sliceAccess = sliceAccesses[ slice ];
		}

Just for getting one random access to one slice of the data, below loop "touches" all slices:

for ( int i = 0; i < slices.length; ++i )
	sliceAccesses[ i ] = slices[ i ].randomAccess();

The issue is that with the current implementation of randomAccess() this triggers the cellLoader to load some data such that it loads data from all slices, which can be a real problem if data access is expensive.

tpietzsch added a commit that referenced this issue Feb 8, 2023
Previously the CellRandomAccess would link Cell data to the Type as soon
as the RA is positioned in a new Cell. Specifically, for LazyCellImgs
this would trigger loading of the Cell (potentially unnecessary).

This addresses #252.
tpietzsch added a commit that referenced this issue Feb 11, 2023
Previously the CellRandomAccess would link Cell data to the Type as soon
as the RA is positioned in a new Cell. Specifically, for LazyCellImgs
this would trigger loading of the Cell (potentially unnecessary).

This addresses #252.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants