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.

Sign up for GitHub

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

Jump to bottom

poi manager: z-position from the scanner is not saved #647

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

poi manager: z-position from the scanner is not saved #647

wants to merge 2 commits into from

Conversation

rmgonzal
Copy link

@rmgonzal rmgonzal commented Mar 24, 2021 •
edited
Loading

Z-position from the scanner is not saved when you add a point of interest (POI)

Description

This bug is affected because POI on the confocal image only saves the x, y position into the roi_origin, and since, by default, the values are set to zero. Since the z position is not saved into the POI file (which should be a fixed value) and take it from ROI origin.
So simply adding new_pos[2] = self.poimanagerlogic().scanner_position[2] in line 629 "gui/poimanger/poimangui.py" will fix this bug.

Motivation and Context

The main motivation to fix this problem is to have the right focal plane for your POI file. This bug is caused when you select a point by clicking into the poi_manager, by default, the values before clicking are set to zero (np.zeros(3)), so when you select more than one POI the Z position is not stored.

So simply adding new_pos[2] = self.poimanagerlogic().scanner_position[2] line 629 "gui/poimanger/poimangui.py" will fix this bug.

#512 (comment)

How Has This Been Tested?

This has been tested by me in two different confocal setups

You can test this by adding more than one POI and checking the file in the console

In[1]: poimanagerlogic.poi_positions

and the last value should be different from zero (e.g. 5 points same Z pos value).

Out[1]: 
{'poi_20210322102945044728': array([3.67941896e-05, 5.56220471e-05, 5.00000000e-05]),
 'poi_20210322102946548945': array([3.15235204e-05, 3.33681106e-05, 5.00000000e-05]),
 'poi_20210322102947693560': array([7.01750944e-05, 5.21082677e-05, 5.00000000e-05]),
 'poi_20210324133553994647': array([2.09481774e-05, 7.84616136e-05, 5.00000000e-05]),
 'poi_20210324133554617894': array([1.27493587e-05, 5.09370079e-05, 5.00000000e-05])}

Screenshots (only if appropriate, delete if not):

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • I have documented my changes in the changelog (documentation/changelog.md)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added/updated for the module the config example in the docstring of the class accordingly.
  • I have checked that the change does not contain obvious errors (syntax, indentation, mutable default values).
  • I have tested my changes using 'Load all modules' on the default dummy configuration with my changes included.
  • All changed Jupyter notebooks have been stripped of their output cells.

@rmgonzal
Update poimangui.py
11bda2c 
#512 (comment)
This bug is affected because POI on the confocal image only saves the x, y position into the roi_origin, and since, by default, the values are set to zero. Since the z position is not saved into the POI file (which should be a fixed value) and take it from ROI origin.
So simply adding new_pos[2] = self.poimanagerlogic().scanner_position[2] line 629 "gui/poimanger/poimangui.py" will fix this bug.
@rmgonzal rmgonzal changed the title Update poimangui.py poi manager: z-position from the scanner is not saved Mar 24, 2021
@NicolasStaudenmaier NicolasStaudenmaier linked an issue Mar 24, 2021 that may be closed by this pull request
Bug to be fixed: When creating a poi by mouse click in poi manager gui, would creat a poi with its z-axis value is zero. #512
Open
3 tasks
NicolasStaudenmaier
NicolasStaudenmaier reviewed Mar 24, 2021
View reviewed changes
Copy link
Contributor

@NicolasStaudenmaier NicolasStaudenmaier left a comment

Choose a reason for hiding this comment

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

Looks good to me. I will test it soon.
The question remains if this is a nice solution or if it is better to save the z-position of the scan as well and work (as I assume it is supposed to) with the sample shift (roi_origin) in z. In any case this is a working solution and perfectly fine for me.

gui/poimanager/poimangui.py Outdated
@@ -626,6 +626,8 @@ def create_poi_from_click(self, button, pos):
new_pos = self.poimanagerlogic().roi_origin
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is not needed anymore. Either initialize an empty array here or define the new position in one line like
new_pos = np.array([pos.x(), pos.y(), self.poimanagerlogic().scanner_position[2]])

@Neverhorst
Copy link
Member

@NicolasStaudenmaier You have recognized the problem here. The ROI origin is the anchor for all POIs registered. So in the beginning the "offset" introduced by sample shift etc. is zero, so the absolute position of an POI is simply the POI initial position. If you now moved the ROI (e.g. by tracking a POI or updating the ROI origin manually), the POI position will become the sum of the POI initial position and the ROI origin (now different from [0, 0, 0]).
If you add a new POI with the current absolute scanner position, the origin is substracted from this position to normalize all POIs to zero sample shift/origin.
So, I'm not sure if this solution works if you add POIs by mouse click in the ROI if you introduce sample shifts in between (create ROI -> shift -> new POI -> shift -> new POI -> shift -> ...). Could be that the origin needs to be subtracted or added for X and Y if adding a POI via mouse click inside the scan image... needs to be checked.

In any case please do not introduce a new absolute coordinate (for Z) to be handled by the POI manager. This is conceptually not needed.

@NicolasStaudenmaier
Copy link
Contributor

It behaves well when adding POIs by mouse clicks, introducing a sample shift and repeating the procedure. When creating an POI the anchor position is calculated by subtracting the shift from the absolute position.

So to fix the issue without introducing an absolute z-coordinate in the POI manager, I believe that the add_poi function in the poimanagerlogic needs to be adapted to accept 2D position inputs (x and y) and taking the z-position from the current scanner position. Then x and y are emitted with the signal upon adding a POI by click and the z-position will be assigned from the poimanagerlogic.

@rmgonzal
Copy link
Author

The idea was to solve this issue. The roi_origin is related to another issue (shift of each track ->update POI). Still, in this case, it gets a bit confused because the POI's shift will be in 3D, and when you select the POI's, you click them in a 2D confocal image, so in principle, the Z position is fixed.

So I updated as @NicolasStaudenmaier @Neverhorst suggest

625 # Z position from scanner, X and Y positions from click event
626 new_pos =  np.array([pos.x(), pos.y(), self.poimanagerlogic().scanner_position[2]])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasStaudenmaier NicolasStaudenmaier NicolasStaudenmaier left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@rmgonzal @Neverhorst @NicolasStaudenmaier