-
Notifications
You must be signed in to change notification settings - Fork 10
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
Merge meshes from all labels #285
base: main
Are you sure you want to change the base?
Conversation
…ith a parcellation map
I think I can understand where the problem lies. How would the user encounter this scenario? I can't help but to think that might be more elegant ways. For example, inside parcellationmap fetch method, when it calls volume's fetch method, it could pass it the exhaustive list of MapIndex. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't suggest the following edit:
# in Map.fetch
result = self.volumes[mapindex.volume or 0].fetch(
fragment=mapindex.fragment, label=mapindex.label, all_labels=self.labels, **kwargs
)
Rather than adding parcellation_map as a property to volumeprovider, is it a better idea to pass Map.labels reference (the suggested edit) as a kwarg during runtime (when fetch is called), since all kwargs are passed down.
We could then avoid the strong coupling between volume provider and map. Volume provider is a lower level concept, which should not reference a higher level concept such as map.
@@ -535,7 +535,7 @@ def _fetch_fragment(self, url: str, transform_nm: np.ndarray): | |||
def fetch(self, label: int, fragment: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def fetch(self, label: int, fragment: str): | |
def fetch(self, label: int, fragment: str, *, all_labels=None, **kwargs): |
if label is None: | ||
if hasattr(self, "parcellation_map"): | ||
# providers of a parcellation map volume are monkey-patched | ||
# with a reference to the map. In that case, we can infer | ||
# the set of available labels, but it cannot be inferred | ||
# from the neuroglancer URL only. | ||
labels = self.parcellation_map.labels | ||
else: | ||
raise RuntimeError(f"Fetching from {self.__class__.__name__} requires to specify a mesh label.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if label is None: | |
if hasattr(self, "parcellation_map"): | |
# providers of a parcellation map volume are monkey-patched | |
# with a reference to the map. In that case, we can infer | |
# the set of available labels, but it cannot be inferred | |
# from the neuroglancer URL only. | |
labels = self.parcellation_map.labels | |
else: | |
raise RuntimeError(f"Fetching from {self.__class__.__name__} requires to specify a mesh label.") | |
if label is None: | |
if all_labels is None: | |
raise RuntimeError(f"Fetching from {self.__class__.__name__} requires to specify a mesh label.") | |
# providers of a parcellation map volume are monkey-patched | |
# with a reference to the map. In that case, we can infer | |
# the set of available labels, but it cannot be inferred | |
# from the neuroglancer URL only. | |
labels = all_labels | |
siibra/volumes/parcellationmap.py
Outdated
# allow the providers to query their parcellation map if needed | ||
for p in v._providers.values(): | ||
p.parcellation_map = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# allow the providers to query their parcellation map if needed | |
for p in v._providers.values(): | |
p.parcellation_map = self |
the commits is already merge into main by way of d66c62c ? so perhaps we can close this PR? |
I am not really happy with any of those. It is somewhat a flaw in the creation of the neuroglancer meshes, or in the design of the mesh data format. I think there should be a json on the neuroglancer side which allows to read out the labels available there (without having access to get a director listing from the server). Isn't there such a thing? |
One could argue that, such standard doesn't exist for nifti either. One must use something like In theory, one could go through the segmentation volume, and find all unique labels, but it's not computationally efficient, especially for large volumes. |
@dickscheid and @xgui3783, is it safe to assume, this PR is not relevant for version 1 anymore and it will be revisited along with the refactor of how meshes are handled in siibra-python? |
If a neuroglancer mesh provider is linked to a parcellation map, and no label is given for fetching mesh fragments, collect them across all labels known in the parcellation instead of raising an error.