Skip to content

Commit

Permalink
ArraySubset improvements and fixes for bugs introduced in bdceff4
Browse files Browse the repository at this point in the history
  • Loading branch information
@LDeakin
LDeakin committed Feb 18, 2024
1 parent 1041356 commit 641acbb
Show file tree
Hide file tree
Showing 16 changed files with 626 additions and 176 deletions.
2 changes: 1 addition & 1 deletion BUILD.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ cargo bench -- --baseline baseline

Install `cargo-llvm-cov`
```bash
cargo +stable install cargo-llvm-cov --locked
cargo +nightly install cargo-llvm-cov --locked
```

Generate a HTML report
Expand Down
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Add `array::ArrayView`
- Add `Array::retrieve_chunk{_subset}_into_array_view_opt` and `retrieve_array_subset_into_array_view`
- Add `array::unsafe_cell_slice::UnsafeCellSlice::len()`
- Add `{Array,Chunk}Representation::dimensionality()`
- Add `ArraySubset::new_empty()` and `ArraySubset::is_empty()`
- Add missing `IncompatibleArraySubsetAndShapeError::new()`
- Add more array tests

### Changed
- Dependency bumps
Expand Down Expand Up @@ -73,6 +77,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- **Breaking**: Remove non-default store lock constructors
- **Breaking**: Remove unused `storage::store::{Readable,Writable,ReadableWritable,Listable}Store`

### Fixed
- `Array::retrieve_array_subset` and variants now correctly return the fill value if the array subset does not intersect any chunks
- **Breaking**: `ArraySubset::end_inc` now returns an `Option`, which is `None` for an empty array subset
- Add missing input validation to some `partial_decode` methods

## [0.11.6] - 2024-02-06

### Added
Expand Down
293 changes: 229 additions & 64 deletions src/array.rs
View file

Large diffs are not rendered by default.

6 changes: 6 additions & 0 deletions src/array/array_representation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ where
&self.array_shape
}

/// Return the dimensionality of the array.
#[must_use]
pub fn dimensionality(&self) -> usize {
self.array_shape.len()
}

/// Return the shape as an [`ArrayShape`] ([`Vec<u64>`]).
#[must_use]
pub fn shape_u64(&self) -> ArrayShape {
Expand Down
129 changes: 96 additions & 33 deletions src/array/array_sync_readable.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ use crate::{
use super::{
codec::{
options::DecodeOptionsBuilder, ArrayCodecTraits, ArrayPartialDecoderTraits,
ArrayToBytesCodecTraits, DecodeOptions, PartialDecoderOptions, StoragePartialDecoder,
ArrayToBytesCodecTraits, CodecError, DecodeOptions, PartialDecoderOptions,
StoragePartialDecoder,
},
concurrency::calc_concurrent_limits,
transmute_from_bytes_vec,
Expand Down Expand Up @@ -281,10 +282,18 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
pub fn retrieve_chunk_into_array_view_opt(
&self,
chunk_indices: &[u64],
array_view: ArrayView,
array_view: &ArrayView,
options: &DecodeOptions,
) -> Result<(), ArrayError> {
let chunk_representation = self.chunk_array_representation(chunk_indices)?;
let chunk_shape_u64 = chunk_representation.shape_u64();
if chunk_shape_u64 != array_view.subset().shape() {
return Err(ArrayError::InvalidArraySubset(
array_view.subset().clone(),
chunk_shape_u64,
));
}

let storage_handle = Arc::new(StorageHandle::new(self.storage.clone()));
let storage_transformer = self
.storage_transformers()
Expand All @@ -309,8 +318,8 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
// fill array_view with fill value
let contiguous_indices = unsafe {
array_view
.subset
.contiguous_linearised_indices_unchecked(array_view.shape)
.subset()
.contiguous_linearised_indices_unchecked(array_view.array_shape())
};
let element_size = chunk_representation.element_size() as u64;
let length =
Expand All @@ -331,6 +340,20 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
}
}

/// Retrieve a chunk and output into an existing array (default options).
#[allow(clippy::missing_panics_doc, clippy::missing_errors_doc)]
pub fn retrieve_chunk_into_array_view(
&self,
chunk_indices: &[u64],
array_view: &ArrayView,
) -> Result<(), ArrayError> {
self.retrieve_chunk_into_array_view_opt(
chunk_indices,
array_view,
&DecodeOptions::default(),
)
}

/// Retrieve a subset of a chunk and output into an existing array.
///
/// # Errors
Expand All @@ -343,18 +366,17 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
&self,
chunk_indices: &[u64],
chunk_subset: &ArraySubset,
array_view: ArrayView,
array_view: &ArrayView,
options: &DecodeOptions,
) -> Result<(), ArrayError> {
let chunk_representation = self.chunk_array_representation(chunk_indices)?;

// Decode the chunk/chunk subset
if chunk_subset.shape() != array_view.subset.shape() {
if chunk_subset.shape() != array_view.subset().shape() {
return Err(ArrayError::InvalidArraySubset(
array_view.subset.clone(),
chunk_subset.shape().to_vec(),
chunk_subset.clone(),
array_view.subset().shape().to_vec(),
));
}

let chunk_representation = self.chunk_array_representation(chunk_indices)?;
if chunk_subset.shape() == chunk_representation.shape_u64() {
self.retrieve_chunk_into_array_view_opt(chunk_indices, array_view, options)
} else {
Expand All @@ -374,6 +396,22 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
}
}

/// Retrieve a subset of a chunk and output into an existing array (default options).
#[allow(clippy::missing_panics_doc, clippy::missing_errors_doc)]
pub fn retrieve_chunk_subset_into_array_view(
&self,
chunk_indices: &[u64],
chunk_subset: &ArraySubset,
array_view: &ArrayView,
) -> Result<(), ArrayError> {
self.retrieve_chunk_subset_into_array_view_opt(
chunk_indices,
chunk_subset,
array_view,
&DecodeOptions::default(),
)
}

/// Read and decode the chunks at `chunks` into their bytes.
///
/// # Errors
Expand Down Expand Up @@ -421,18 +459,24 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
});
// FIXME: constrain concurrency based on codec
let indices = chunks.indices();
let chunk0_subset = self.chunk_subset(chunks.start())?;
rayon_iter_concurrent_limit::iter_concurrent_limit!(
options.concurrent_limit(),
1,
indices.into_par_iter(),
try_for_each,
|chunk_indices| {
let chunk_subset = self.chunk_subset(&chunk_indices)?;
let array_view_subset = unsafe {
chunk_subset.relative_to_unchecked(chunk0_subset.start())
};
self.retrieve_chunk_into_array_view_opt(
&chunk_indices,
ArrayView::new(
&ArrayView::new(
unsafe { output_slice.get() },
array_subset.shape(),
&array_subset,
),
array_view_subset,
)
.map_err(|err| CodecError::from(err.to_string()))?,
options,
)
}
Expand Down Expand Up @@ -554,19 +598,22 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
// Retrieve chunk bytes
let num_chunks = chunks.num_elements();
match num_chunks {
0 => Ok(vec![]),
0 => Ok(self
.fill_value()
.as_ne_bytes()
.repeat(array_subset.num_elements_usize())),
1 => {
let chunk_indices = chunks.start();
let chunk_subset = self.chunk_subset(chunk_indices)?;
if &chunk_subset == array_subset {
// Single chunk fast path if the array subset domain matches the chunk domain
self.retrieve_chunk_opt(chunk_indices, options)
} else {
let chunk_subset_in_array_subset =
unsafe { chunk_subset.relative_to_unchecked(array_subset.start()) };
let array_subset_in_chunk_subset =
unsafe { array_subset.relative_to_unchecked(chunk_subset.start()) };
self.retrieve_chunk_subset_opt(
chunk_indices,
&chunk_subset_in_array_subset,
&array_subset_in_chunk_subset,
options,
)
}
Expand Down Expand Up @@ -617,12 +664,13 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
let array_view = ArrayView::new(
unsafe { output.get() },
array_subset.shape(),
&array_view_subset,
);
array_view_subset,
)
.map_err(|err| CodecError::from(err.to_string()))?;
self.retrieve_chunk_subset_into_array_view_opt(
&chunk_indices,
&chunk_subset,
array_view,
&array_view,
&codec_options,
)
}
Expand All @@ -642,16 +690,16 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
///
/// # Panics
/// Panics if an offset is larger than `usize::MAX`.
pub fn retrieve_array_subset_into_array_view(
pub fn retrieve_array_subset_into_array_view_opt(
&self,
array_subset: &ArraySubset,
array_view: &ArrayView,
options: &DecodeOptions,
) -> Result<(), ArrayError> {
if array_subset.dimensionality() != self.chunk_grid().dimensionality() {
if array_subset.shape() != array_view.subset().shape() {
return Err(ArrayError::InvalidArraySubset(
array_subset.clone(),
self.shape().to_vec(),
array_view.subset().shape().to_vec(),
));
}

Expand All @@ -674,10 +722,11 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
if &chunk_subset == array_subset {
// Single chunk fast path if the array subset domain matches the chunk domain
let array_view_subset =
unsafe { chunk_subset.relative_to_unchecked(array_view.subset.start()) };
unsafe { chunk_subset.relative_to_unchecked(array_subset.start()) };
self.retrieve_chunk_into_array_view_opt(
chunk_indices,
array_view.subset_view(&array_view_subset),
&unsafe { array_view.subset_view(&array_view_subset) }
.map_err(|err| CodecError::from(err.to_string()))?,
options,
)
} else {
Expand All @@ -687,13 +736,13 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
chunk_subset_in_array_subset.relative_to_unchecked(chunk_subset.start())
};
let array_view_subset = unsafe {
chunk_subset_in_array_subset
.relative_to_unchecked(array_view.subset.start())
chunk_subset_in_array_subset.relative_to_unchecked(array_subset.start())
};
self.retrieve_chunk_subset_into_array_view_opt(
chunk_indices,
&chunk_subset,
array_view.subset_view(&array_view_subset),
&unsafe { array_view.subset_view(&array_view_subset) }
.map_err(|err| CodecError::from(err.to_string()))?,
options,
)
}
Expand All @@ -717,7 +766,6 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
// println!("self_concurrent_limit {self_concurrent_limit:?} codec_concurrent_limit {codec_concurrent_limit:?}"); // FIXME: log this

{
// FIXME: Constrain concurrency here based on parallelism internally vs externally
let indices = chunks.indices();
iter_concurrent_limit!(
self_concurrent_limit,
Expand All @@ -733,12 +781,13 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
};
let array_view_subset = unsafe {
chunk_subset_in_array_subset
.relative_to_unchecked(array_view.subset.start())
.relative_to_unchecked(array_subset.start())
};
self.retrieve_chunk_subset_into_array_view_opt(
&chunk_indices,
&chunk_subset,
array_view.subset_view(&array_view_subset),
&unsafe { array_view.subset_view(&array_view_subset) }
.map_err(|err| CodecError::from(err.to_string()))?,
&codec_options,
)
}
Expand All @@ -749,6 +798,20 @@ impl<TStorage: ?Sized + ReadableStorageTraits + 'static> Array<TStorage> {
}
}

/// Retrieve an array subset into an array view (default options).
#[allow(clippy::missing_errors_doc)]
pub fn retrieve_array_subset_into_array_view(
&self,
array_subset: &ArraySubset,
array_view: &ArrayView,
) -> Result<(), ArrayError> {
self.retrieve_array_subset_into_array_view_opt(
array_subset,
array_view,
&DecodeOptions::default(),
)
}

/// Read and decode the `array_subset` of array into its bytes.
///
/// # Errors
Expand Down
Loading

0 comments on commit 641acbb

Please sign in to comment.