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

Merge meshes from all labels #285

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Merge meshes from all labels #285

wants to merge 3 commits into from

Conversation

dickscheid
Copy link
Contributor

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.

@dickscheid dickscheid requested a review from xgui3783 February 24, 2023 14:36
@xgui3783
Copy link
Member

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.

Copy link
Member

@xgui3783 xgui3783 left a 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):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def fetch(self, label: int, fragment: str):
def fetch(self, label: int, fragment: str, *, all_labels=None, **kwargs):

Comment on lines +559 to +567
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.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

Comment on lines 160 to 162
# allow the providers to query their parcellation map if needed
for p in v._providers.values():
p.parcellation_map = self
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# allow the providers to query their parcellation map if needed
for p in v._providers.values():
p.parcellation_map = self

@xgui3783
Copy link
Member

the commits is already merge into main by way of d66c62c ?

so perhaps we can close this PR?

@dickscheid
Copy link
Contributor Author

dickscheid commented Apr 20, 2023

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?

@xgui3783
Copy link
Member

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 np.unique to find all labels.

In theory, one could go through the segmentation volume, and find all unique labels, but it's not computationally efficient, especially for large volumes.

@AhmetNSimsek AhmetNSimsek marked this pull request as draft May 31, 2023 14:08
@AhmetNSimsek
Copy link
Collaborator

@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?

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.

3 participants