-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add oriented mesh production #4
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
Conversation
…e' into feature/CC-316-new
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.
Looks good overall, thanks! Some comments from reading the code, going to try it out now as well
Edit - I tried it, looks nice :) I think we need to resolve with CZI what the state should actually be in this case. And then once we know what the "ideal" state for this data would be we can go backwards and discuss whether the backend should handle that or the Python generation. Likely some combo of both would be needed.
|
||
return state_generator.generate_oriented_point_mesh_layer(**args) | ||
|
||
def _has_mesh(self, path: 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.
I think this should either be more generic and take the shape into account for the names that are searched for, or the function should be named differently. Like _has_oriented_point_mesh
for e.g.
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.
sidenote -- it might be clearer if the function took an input shape as an arg and called path.replace(input_shape.lower(), "orientedmesh"). I'd also probably be inclined to run this after splitting on "_" and do it on the last one or similar in case for some reason the input had "_segmentationmask" for e.g. in it but not just at the end because then it wouldn't match correctly. It seems unlikely to happen but I feel you never know with filenames
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.
Whatever the decision on that it would need to be reflected in visualization_precompute
L154
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.
Hey @seankmartin I did all the modifications you suggested, one not though about this method. I took a long time to try to avoid it, by linking the source configuration with the target one, to avoid to have to rely on this "juggling" in names. However, there is nothing that clearly relates the output layer configuration with the source configuration. The detection has to pass by considering sub-strings inclusion, and at the end, it doesn't feel more stable or "stronger" that this one that only relies on switching the "kind" of produced layer to check for mesh existence.
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.
Hey @aranega don't worry, thank you for taking a look at that and trying to make the change!
Having the name changed to be more specific to oriented points and changing the last occurence of the string is much nicer so this is all good
Small note, the |
oriented points mesh existence
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.
Looks great, thank you!
PR opened here chanzuckerberg#511 |
…ackend into feature/CC-316-new
… paths from point to mesh
Closing as made PR to main repo |
Description
Adds the production of oriented mesh layers if meshes related to oriented point are found in the input bucket.
The input bucket needs to have the meshes in the
mesh_source
directory, located at the root of the dataset folder, e.g: