From 85cb1513190bdfa6e369be0ef8e8b7ed1799a1a9 Mon Sep 17 00:00:00 2001 From: Niels Dekker Date: Fri, 16 Aug 2024 20:25:03 +0200 Subject: [PATCH 1/3] STYLE: Move ComponentType conversion functions to unnamed namespace The functions `tensorstoreToITKComponentType` and `itkToTensorstoreComponentType` are only used internally in their cxx file. Following C++ Core Guidelines, May 11, 2024, "Use an unnamed (anonymous) namespace for all internal/non-exported entities", http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf22-use-an-unnamed-anonymous-namespace-for-all-internalnon-exported-entities --- src/itkOMEZarrNGFFImageIO.cxx | 198 +++++++++++++++++----------------- 1 file changed, 99 insertions(+), 99 deletions(-) diff --git a/src/itkOMEZarrNGFFImageIO.cxx b/src/itkOMEZarrNGFFImageIO.cxx index 8dc9167..7b7ce43 100644 --- a/src/itkOMEZarrNGFFImageIO.cxx +++ b/src/itkOMEZarrNGFFImageIO.cxx @@ -36,105 +36,6 @@ namespace itk { namespace { -template -void -ReadFromStore(const tensorstore::TensorStore<> & store, const ImageIORegion & storeIORegion, TPixel * buffer) -{ - if (store.domain().num_elements() == storeIORegion.GetNumberOfPixels()) - { - // Read the entire available voxel region. - // Allow tensorstore to perform any axis permutations or other index operations - // to map from store axes to ITK image axes. - auto arr = tensorstore::Array(buffer, store.domain().shape(), tensorstore::c_order); - tensorstore::Read(store, tensorstore::UnownedToShared(arr)).value(); - } - else - { - // Read a requested voxel subregion. - // We cannot infer axis permutations by matching requested axis sizes. - // Therefore we assume that tensorstore axes are in "C-style" order - // with the last index as the fastest moving axis, aka "z,y,x" order, - // and must inverted to match ITK's "Fortran-style" order of axis indices - // with the first index as the fastest moving axis, aka "x,y,z" order. - // "C-style" is generally the default layout for new tensorstore arrays. - // Refer to https://google.github.io/tensorstore/driver/zarr/index.html#json-driver/zarr.metadata.order - // - // In the future this may be extended to permute axes based on - // OME-Zarr NGFF axis labels. - const auto dimension = store.rank(); - std::vector indices(dimension); - std::vector sizes(dimension); - for (size_t dim = 0; dim < dimension; ++dim) - { - // Input IO region is assumed to already be reversed from ITK requested region - // to match assumed C-style Zarr storage - indices[dim] = storeIORegion.GetIndex(dim); - sizes[dim] = storeIORegion.GetSize(dim); - } - auto indexDomain = tensorstore::IndexDomainBuilder(dimension).origin(indices).shape(sizes).Finalize().value(); - - auto arr = tensorstore::Array(buffer, indexDomain.shape(), tensorstore::c_order); - auto indexedStore = store | tensorstore::AllDims().SizedInterval(indices, sizes); - tensorstore::Read(indexedStore, tensorstore::UnownedToShared(arr)).value(); - } -} - -// Update an existing "read" specification for an "http" driver to retrieve remote files. -// Note that an "http" driver specification may operate on an HTTP or HTTPS connection. -void -MakeKVStoreHTTPDriverSpec(nlohmann::json & spec, const std::string & fullPath) -{ - // Decompose path into a base URL and reference subpath according to TensorStore HTTP KVStore driver spec - // https://google.github.io/tensorstore/kvstore/http/index.html - spec["kvstore"] = { { "driver", "http" } }; - - // Naively decompose the URL into "base" and "resource" components. - // Generally assumes that the spec will only be used once to access a specific resource. - // For example, the URL "http://localhost/path/to/resource.json" will be split - // into components "http://localhost/path/to" and "resource.json". - // - // Could be revisited for a better root "base_url" at the top level allowing acces - // to multiple subpaths. For instance, decomposing the example above into - // "http://localhost/" and "path/to/resource.json" would allow for a given HTTP spec - // to be more easily reused with different subpaths. - // - spec["kvstore"]["base_url"] = fullPath.substr(0, fullPath.find_last_of("/")); - spec["kvstore"]["path"] = fullPath.substr(fullPath.find_last_of("/") + 1); -} - -} // namespace - -thread_local tensorstore::Context tsContext = tensorstore::Context::Default(); - -OMEZarrNGFFImageIO::OMEZarrNGFFImageIO() -{ - this->AddSupportedWriteExtension(".zarr"); - this->AddSupportedWriteExtension(".zr2"); - this->AddSupportedWriteExtension(".zr3"); - this->AddSupportedWriteExtension(".zip"); - this->AddSupportedWriteExtension(".memory"); - - this->AddSupportedReadExtension(".zarr"); - this->AddSupportedReadExtension(".zr2"); - this->AddSupportedReadExtension(".zr3"); - this->AddSupportedReadExtension(".zip"); - this->AddSupportedWriteExtension(".memory"); - - this->Self::SetCompressor(""); - this->Self::SetMaximumCompressionLevel(9); - this->Self::SetCompressionLevel(2); -} - - -void -OMEZarrNGFFImageIO::PrintSelf(std::ostream & os, Indent indent) const -{ - Superclass::PrintSelf(os, indent); - os << indent << "DatasetIndex: " << m_DatasetIndex << std::endl; - os << indent << "TimeIndex: " << m_TimeIndex << std::endl; - os << indent << "ChannelIndex: " << m_ChannelIndex << std::endl; -} - IOComponentEnum tensorstoreToITKComponentType(const tensorstore::DataType dtype) { @@ -246,6 +147,105 @@ itkToTensorstoreComponentType(const IOComponentEnum itkComponentType) } } +template +void +ReadFromStore(const tensorstore::TensorStore<> & store, const ImageIORegion & storeIORegion, TPixel * buffer) +{ + if (store.domain().num_elements() == storeIORegion.GetNumberOfPixels()) + { + // Read the entire available voxel region. + // Allow tensorstore to perform any axis permutations or other index operations + // to map from store axes to ITK image axes. + auto arr = tensorstore::Array(buffer, store.domain().shape(), tensorstore::c_order); + tensorstore::Read(store, tensorstore::UnownedToShared(arr)).value(); + } + else + { + // Read a requested voxel subregion. + // We cannot infer axis permutations by matching requested axis sizes. + // Therefore we assume that tensorstore axes are in "C-style" order + // with the last index as the fastest moving axis, aka "z,y,x" order, + // and must inverted to match ITK's "Fortran-style" order of axis indices + // with the first index as the fastest moving axis, aka "x,y,z" order. + // "C-style" is generally the default layout for new tensorstore arrays. + // Refer to https://google.github.io/tensorstore/driver/zarr/index.html#json-driver/zarr.metadata.order + // + // In the future this may be extended to permute axes based on + // OME-Zarr NGFF axis labels. + const auto dimension = store.rank(); + std::vector indices(dimension); + std::vector sizes(dimension); + for (size_t dim = 0; dim < dimension; ++dim) + { + // Input IO region is assumed to already be reversed from ITK requested region + // to match assumed C-style Zarr storage + indices[dim] = storeIORegion.GetIndex(dim); + sizes[dim] = storeIORegion.GetSize(dim); + } + auto indexDomain = tensorstore::IndexDomainBuilder(dimension).origin(indices).shape(sizes).Finalize().value(); + + auto arr = tensorstore::Array(buffer, indexDomain.shape(), tensorstore::c_order); + auto indexedStore = store | tensorstore::AllDims().SizedInterval(indices, sizes); + tensorstore::Read(indexedStore, tensorstore::UnownedToShared(arr)).value(); + } +} + +// Update an existing "read" specification for an "http" driver to retrieve remote files. +// Note that an "http" driver specification may operate on an HTTP or HTTPS connection. +void +MakeKVStoreHTTPDriverSpec(nlohmann::json & spec, const std::string & fullPath) +{ + // Decompose path into a base URL and reference subpath according to TensorStore HTTP KVStore driver spec + // https://google.github.io/tensorstore/kvstore/http/index.html + spec["kvstore"] = { { "driver", "http" } }; + + // Naively decompose the URL into "base" and "resource" components. + // Generally assumes that the spec will only be used once to access a specific resource. + // For example, the URL "http://localhost/path/to/resource.json" will be split + // into components "http://localhost/path/to" and "resource.json". + // + // Could be revisited for a better root "base_url" at the top level allowing acces + // to multiple subpaths. For instance, decomposing the example above into + // "http://localhost/" and "path/to/resource.json" would allow for a given HTTP spec + // to be more easily reused with different subpaths. + // + spec["kvstore"]["base_url"] = fullPath.substr(0, fullPath.find_last_of("/")); + spec["kvstore"]["path"] = fullPath.substr(fullPath.find_last_of("/") + 1); +} + +} // namespace + +thread_local tensorstore::Context tsContext = tensorstore::Context::Default(); + +OMEZarrNGFFImageIO::OMEZarrNGFFImageIO() +{ + this->AddSupportedWriteExtension(".zarr"); + this->AddSupportedWriteExtension(".zr2"); + this->AddSupportedWriteExtension(".zr3"); + this->AddSupportedWriteExtension(".zip"); + this->AddSupportedWriteExtension(".memory"); + + this->AddSupportedReadExtension(".zarr"); + this->AddSupportedReadExtension(".zr2"); + this->AddSupportedReadExtension(".zr3"); + this->AddSupportedReadExtension(".zip"); + this->AddSupportedWriteExtension(".memory"); + + this->Self::SetCompressor(""); + this->Self::SetMaximumCompressionLevel(9); + this->Self::SetCompressionLevel(2); +} + + +void +OMEZarrNGFFImageIO::PrintSelf(std::ostream & os, Indent indent) const +{ + Superclass::PrintSelf(os, indent); + os << indent << "DatasetIndex: " << m_DatasetIndex << std::endl; + os << indent << "TimeIndex: " << m_TimeIndex << std::endl; + os << indent << "ChannelIndex: " << m_ChannelIndex << std::endl; +} + // Returns TensorStore KvStore driver name appropriate for this path. // Options are file, zip. TODO: http, gcs (GoogleCouldStorage), etc. std::string From e760faa9d6f37c2640af5c015384d89141e83d98 Mon Sep 17 00:00:00 2001 From: Niels Dekker Date: Fri, 16 Aug 2024 21:25:37 +0200 Subject: [PATCH 2/3] STYLE: Remove duplicate `READ_ELEMENT_IF(float)` call from Read(void *) This line of code can just be removed, as was confirmed by Tom Birdsong at https://github.com/InsightSoftwareConsortium/ITKIOOMEZarrNGFF/pull/67#discussion_r1720236212 --- src/itkOMEZarrNGFFImageIO.cxx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/itkOMEZarrNGFFImageIO.cxx b/src/itkOMEZarrNGFFImageIO.cxx index 7b7ce43..ca354d9 100644 --- a/src/itkOMEZarrNGFFImageIO.cxx +++ b/src/itkOMEZarrNGFFImageIO.cxx @@ -637,7 +637,6 @@ OMEZarrNGFFImageIO::Read(void * buffer) if (false) { } - READ_ELEMENT_IF(float) READ_ELEMENT_IF(int8_t) READ_ELEMENT_IF(uint8_t) READ_ELEMENT_IF(int16_t) From 77a2f45224dd1ccf00fa1b49b94136141663927e Mon Sep 17 00:00:00 2001 From: Niels Dekker Date: Fri, 16 Aug 2024 21:34:36 +0200 Subject: [PATCH 3/3] STYLE: Replace READ_ELEMENT_IF with TryToReadFromStore variadic template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaced the original `READ_ELEMENT_IF` macro with a variadic template, using a C++17 fold-expression. Following C++ Core Guidelines, May 11, 2024, "Don’t use macros for program text manipulation", https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-macros --- src/itkOMEZarrNGFFImageIO.cxx | 54 ++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 23 deletions(-) diff --git a/src/itkOMEZarrNGFFImageIO.cxx b/src/itkOMEZarrNGFFImageIO.cxx index ca354d9..a8317a3 100644 --- a/src/itkOMEZarrNGFFImageIO.cxx +++ b/src/itkOMEZarrNGFFImageIO.cxx @@ -190,6 +190,33 @@ ReadFromStore(const tensorstore::TensorStore<> & store, const ImageIORegion & st } } +// Reads from the store if the specified pixel type and the ITK component type match. +template +bool +ReadFromStoreIfTypesMatch(const IOComponentEnum componentType, + const tensorstore::TensorStore<> & store, + const ImageIORegion & storeIORegion, + void * buffer) +{ + if (tensorstoreToITKComponentType(tensorstore::dtype_v) == componentType) + { + ReadFromStore(store, storeIORegion, static_cast(buffer)); + return true; + } + return false; +} + +// Tries to read from the specified store, trying any of the specified pixel types. +template +bool +TryToReadFromStore(const IOComponentEnum componentType, + const tensorstore::TensorStore<> & store, + const ImageIORegion & storeIORegion, + void * buffer) +{ + return (ReadFromStoreIfTypesMatch(componentType, store, storeIORegion, buffer) || ...); +} + // Update an existing "read" specification for an "http" driver to retrieve remote files. // Note that an "http" driver specification may operate on an HTTP or HTTPS connection. void @@ -601,14 +628,6 @@ OMEZarrNGFFImageIO::ReadImageInformation() ReadArrayMetadata(std::string(this->GetFileName()) + "/" + path, driver); } -// We call tensorstoreToITKComponentType for each type. -// Hopefully compiler will optimize it away via constant propagation and inlining. -#define READ_ELEMENT_IF(typeName) \ - else if (tensorstoreToITKComponentType(tensorstore::dtype_v) == this->GetComponentType()) \ - { \ - ReadFromStore(store, storeIORegion, reinterpret_cast(buffer)); \ - } - void OMEZarrNGFFImageIO::Read(void * buffer) { @@ -634,22 +653,11 @@ OMEZarrNGFFImageIO::Read(void * buffer) << storeIORegion; } - if (false) + if (const IOComponentEnum componentType{ this->GetComponentType() }; + !TryToReadFromStore( + componentType, store, storeIORegion, buffer)) { - } - READ_ELEMENT_IF(int8_t) - READ_ELEMENT_IF(uint8_t) - READ_ELEMENT_IF(int16_t) - READ_ELEMENT_IF(uint16_t) - READ_ELEMENT_IF(int32_t) - READ_ELEMENT_IF(uint32_t) - READ_ELEMENT_IF(int64_t) - READ_ELEMENT_IF(uint64_t) - READ_ELEMENT_IF(float) - READ_ELEMENT_IF(double) - else - { - itkExceptionMacro("Unsupported component type: " << GetComponentTypeAsString(this->GetComponentType())); + itkExceptionMacro("Unsupported component type: " << GetComponentTypeAsString(componentType)); } }