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

Add a getter for internal voxel maps #6

Closed
ma3ke opened this issue Apr 18, 2024 · 2 comments
Closed

Add a getter for internal voxel maps #6

ma3ke opened this issue Apr 18, 2024 · 2 comments

Comments

@ma3ke
Copy link
Contributor

ma3ke commented Apr 18, 2024

The containment determination procedure keeps track of a set of internal voxel maps that represent which spaces are segmented into what compartments. Currently, these can be accessed as shown in the following example.

from mdvcontainment import Containment
containment = Containment(u, resolution)
# Access the voxel map where the value of each voxel holds the compartment index of the space it occupies.
label_array = containment.data["relabeled_combined_label_array"]

Though it is very helpful that the data field on the Containment object allows access to this and other internal voxel maps, it is a poorly documented and somewhat hazardous interface.

  • It is unclear from the outside (that is, without going through and understanding the source code) which of the items in the data dict fulfill which function.
  • Furthermore, it is not clear what guarantees are held by these items.
  • The interface feels fragile to me, since I need to access items that I consider to be well within the realm of implementation details. It would not surprise me if that internal organization changes subtly in the future, which could invalidate an approach that happened to work before.

Suggestion

I think it would be wonderful if at least the "relabeled_combined_label_array" could be exposed as a proper getter method.

This would aid in clarity since the getter can be documented to reflect what value it provides, what it can guarantee, and how to use it correctly. It takes out guesswork in which of the voxel maps is appropriate for one's application.
Also, in the event the internal organization is ever changed, the getter could still be kept valid such that the outside interface is not affected.

Next steps

We need to decide on the voxel maps that are useful to expose through getters. As far as I'm concerned, the relabeled combined label array can be the only one that is exposed. But perhaps there are other items in the data dict that are similarly ready for outside exposure.

I would also be up for submitting a PR for this change, if you want.

@BartBruininks
Copy link
Owner

The getter has been implemented in the rework.

VoxelContainment.get_voxel_mask()

@ma3ke
Copy link
Contributor Author

ma3ke commented Jul 5, 2024

Lovely! I have used it and it works wonderfully. Thanks :)

@ma3ke ma3ke closed this as completed Jul 5, 2024
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

No branches or pull requests

2 participants