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

photom v2 roi selection for ablation demo #185

Merged
merged 34 commits into from
Jul 26, 2024

Conversation

aaronalvarezcz
Copy link

@aaronalvarezcz aaronalvarezcz commented Jun 11, 2024

This PR adds the feature that compromised the bulk of my work as an intern. It improves the PhotoM drawing mode capabilities, allowing the user to draw shapes/ROIs, fill them with patterns, and ablate over those patterns by interfacing with the hardware.

Future Enhancement
A good extension of this project is integrating it with on-the-fly segmentations and turning the segmentations into ROIs so they can be ablated. This will further automate and streamline the workflow for ablating with PhotoM.

Copy link
Contributor

@edyoshikun edyoshikun left a comment

Choose a reason for hiding this comment

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

This is a great demo of how we can draw and ablate some points. I wasn't able to check what the ablate method does. My suggestion is to get the copylot logger and use the logging.logger.debug or logging.logger.info to have this spit out information. For example, I would at least logger.info('Ablation Button Pressed') and as logger.debug(f'Ablation coordinates {coordinates of the shape to ablate}))

I think this will be clear when you add the docstring, but it's not clear what does bidirectional means.

Some things that would be nice to have is the logic to delete the specific shapes. So for example, have a method/wrapper that pops() or removes() specific shapes. You can add a button called Delete in the GUI that if the shape is selected from the dropdown and you click delete that it gets removed.

copylot/assemblies/photom/demo/demo_roi.py Outdated Show resolved Hide resolved
copylot/assemblies/photom/demo/demo_roi.py Outdated Show resolved Hide resolved
copylot/assemblies/photom/demo/demo_roi.py Outdated Show resolved Hide resolved
copylot/assemblies/photom/demo/demo_roi.py Outdated Show resolved Hide resolved
@edyoshikun
Copy link
Contributor

This one is minor but if the spacing is too big the behaviour should be that it at least has 1 point in the ROI and also throw some warn and/or logger.warning

@edyoshikun edyoshikun mentioned this pull request Jul 5, 2024
Added:
- drawing mode group in control window, includes roi selection and pattern dropdown, pattern spacing boxes, and deletion of shapes
- scrollable widget behavior for laser control window
- attributes to LaserMarkerWindow class to contain shapes

Changed:
- how displaying shapes are handled. shape/drawings are displayed by calling self.update() now
- shapesUpdate signal to notify of new shapes added
- updated mouse event handling to track and store Shape objects
copylot/assemblies/photom/gui/windows.py Outdated Show resolved Hide resolved
copylot/assemblies/photom/gui/windows.py Outdated Show resolved Hide resolved
copylot/assemblies/photom/gui/windows.py Outdated Show resolved Hide resolved
copylot/assemblies/photom/gui/windows.py Outdated Show resolved Hide resolved
@edyoshikun
Copy link
Contributor

When I draw the different ROIs, and click apply. It only moves to position [0] and it freezes. It doesn't print the next positions. Do you have an idea on why that happens?

Copy link
Contributor

@edyoshikun edyoshikun left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for these new features @aaronalvarezcz. This looks great, so I am merging it.

@edyoshikun edyoshikun merged commit bc79429 into czbiohub-sf:photom_v2 Jul 26, 2024
1 check 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