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

STYLE: Replace thread_local variables with m_TensorStoreData data member #71

Conversation

N-Dekker
Copy link
Collaborator

It appears preferable that multiple OMEZarrNGFFImageIO instances do not share their TensorStore context and store. With this commit, each OMEZarrNGFFImageIO instance will have its own context and store, stored in its m_TensorStoreData data member.

The m_TensorStoreData data member hides the TensorStore data from the interface, as it is a pointer to a forward-declared struct, similar to the "Pimpl" idiom.

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good on a glance.

What drives your interest in this remote module?

@N-Dekker
Copy link
Collaborator Author

What drives your interest in this remote module?

Good question 😃 One of our aims for the elastix project is to add OME-Zarr support! At least for the input images. We are hoping to get it working by using your remote module!

@N-Dekker
Copy link
Collaborator Author

So... what inspired you to make this module?

@dzenanz
Copy link
Member

dzenanz commented Aug 27, 2024

Adding support for distributed image registration, as part of a grant-funded project.

@N-Dekker
Copy link
Collaborator Author

Did you write it all from scratch, or is there helpful example code of how to use tensorstore in C++, that you could use?

Like, how did you figure this out?

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();

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();

I'm impressed!

@dzenanz
Copy link
Member

dzenanz commented Aug 27, 2024

I started from scratch. I was looking at unit tests in tensorstore, and to a lesser degree at their Python documentation. TensorStore authors also helped me a lot. Take a look at issues opened by me:
https://github.com/google/tensorstore/issues?q=is%3Aissue+author%3Adzenanz+sort%3Acreated-asc
Most of them had pretty long discussions.

@N-Dekker N-Dekker force-pushed the Replace-thread_local-tensorstore-variables branch from 3dd47f5 to 09b62e9 Compare August 27, 2024 17:38
It appears preferable that multiple OMEZarrNGFFImageIO instances do not share
their TensorStore context and store. With this commit, each OMEZarrNGFFImageIO
instance will have its own context and store, stored in its `m_TensorStoreData`
data member.

The `m_TensorStoreData` data member hides the TensorStore data from the
interface, as it is a pointer to a forward-declared struct, similar to the
"Pimpl" idiom.
@N-Dekker N-Dekker force-pushed the Replace-thread_local-tensorstore-variables branch from 09b62e9 to 0b9ca69 Compare August 27, 2024 20:42
@N-Dekker N-Dekker marked this pull request as ready for review August 27, 2024 22:15
@N-Dekker
Copy link
Collaborator Author

N-Dekker commented Sep 3, 2024

I think this pull request is ready 👍 (GitHub does not allow me to approve my own pull request 🤷.)

@dzenanz dzenanz merged commit 7b52663 into InsightSoftwareConsortium:main Sep 3, 2024
18 checks passed
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

Successfully merging this pull request may close these issues.

2 participants