Skip to content

Drift correction#7

Open
vilim wants to merge 27 commits intomasterfrom
drift_corr_v2
Open

Drift correction#7
vilim wants to merge 27 commits intomasterfrom
drift_corr_v2

Conversation

@vilim
Copy link
Copy Markdown
Member

@vilim vilim commented Jun 30, 2020

No description provided.

@diegoasua diegoasua self-requested a review June 30, 2020 14:58
@diegoasua diegoasua linked an issue Jun 30, 2020 that may be closed by this pull request
Comment thread twop/drift_correction.py
self.extra_planes = Param(11, (1, 500))
self.dz = Param(1.0, (0.1, 20.0), unit="um")
self.xy_th = Param(5.0, (0.1, 20.0), unit="um")
self.z_th = Param(self.dz, (self.dz, self.dz * 4), unit="um")
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

you might have problems with parameters that depend on other parameters (I think here the values will just be copied)

Comment thread twop/drift_correction.py
self.n_frames_ref = Param(10, (1, 500))
self.extra_planes = Param(11, (1, 500))
self.dz = Param(1.0, (0.1, 20.0), unit="um")
self.xy_th = Param(5.0, (0.1, 20.0), unit="um")
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the _th are thresholds? please use longer names

Comment thread twop/drift_correction.py
n_frames_exp = st.n_frames_exp
sigma_k = st.sigma_k
size_k = st.size_k
rp = ReferenceParameters(n_frames_ref=n_frames_ref,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

if all the names are same, you can covert settings to a dictionary (provided by lighparam). Also we should add a possibility to make a dataclass out of a Parameterized

Comment thread twop/drift_correction.py
errors = []
planes = np.size(self.reference, 0)
for i in range(planes):
ref_im = np.squeeze(self.reference[i, :, :])
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

please, no squeezing

Comment thread twop/drift_correction.py
planes = np.size(self.reference, 0)
for i in range(planes):
ref_im = np.squeeze(self.reference[i, :, :])
output = register_translation(ref_im, test_image)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

once you upgrade scikit-image to 0.17 the function has been moved

Comment thread twop/drift_correction.py
z_disp = ind - ((self.reference_params.n_planes - 1) / 2)
vector = vectors[ind]
np.append(vector, z_disp)
vector = self.real_units(vector)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

a more concise way of doing thse would be self.to_real_units(np.r_[vector, z_disp]) without the np.append np.r_ is a shortcut to concatenate

Comment thread twop/drift_correction.py
while not self.stop_event.is_set():
number_of_frames = 0
frame_container = []
while number_of_frames == self.reference_params.n_frames_exp:
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is a bit strange condition? I aussume it will never be fullfilled unless self.reference_params.n_frames_exp is 0, because the loop does not update the self.reference_params

Comment thread twop/power_control.py
self.encoding = encoding
self.port = port
self.device = device
rm = pyvisa.ResourceManager()
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I guess you will have to put this back. For testing I suggest a FakeLaserPowerController class

Comment thread twop/drift_correction.py
w_fov = conv_fact * self.scanning_parameters.voltage_x
return (self.scanning_parameters.n_x / w_fov) / 1000

def save_reference(self, raw_reference):
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think this class should be managing the saving of the reference. Try to constrain it just to do drift correction and nothing else.

Comment thread twop/drift_correction.py
self.input_commands_queues["y"].put((vector[0], self.mov_type))
self.input_commands_queues["z"].put((vector[2], self.mov_type))

def reference_processing(self, input_ref):
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

A tip for speed: once you received the reference, you can immediately fourier transform it and low-pass filter it in fourier space. That way the register_translation will be much faster (see the space argument here https://scikit-image.org/docs/stable/api/skimage.registration.html?highlight=phase_cross_correlation#phase-cross-correlation and the source code of the function)

Comment thread twop/drift_correction.py
self.name = "reference"
self.n_frames_ref = Param(10, (1, 500))
self.extra_planes = Param(11, (1, 500))
self.dz = Param(1.0, (0.1, 20.0), unit="um")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't there already a parameter like this in status? Don't duplicate parameters, move them through queues

Comment thread twop/drift_correction.py
z_th: float = 1
n_frames_exp: int = 5
size_k: int = 0
sigma_k: int = 5
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is k?

Comment thread twop/drift_correction.py
self.calibration_vector = None

def run(self):
while True:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be while not stop_event.is_set() otherwise this process won't close properly. Also make sure it is .join() in the experiment termination function

Comment thread twop/drift_correction.py
self.calibration_vector = [pix_millimeter, pix_millimeter, self.reference_params.dz] # x,y,z cal vect
while not self.stop_event.is_set():
number_of_frames = 0
frame_container = []
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you know the size of the frame container? If so better use a preallocated numpy then index it

Comment thread twop/drift_correction.py
frame_container = []
while number_of_frames == self.reference_params.n_frames_exp:
try:
frame = self.data_queue.get(timeout=0.001)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you get from data_queue does the Saver still get the same data? You have to have into account that the get() method in a multiprocessing.queue not only obtains an element of the queue but also deletes it from the queue.

Comment thread twop/drift_correction.py
out = queue.get(timeout=0.001)
except Empty:
pass
return out
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This while will freeze the process here if nothing is received for whatever reason

Comment thread twop/drift_correction.py
def calculate_fov(self):
# calculate pix per millimeters
# formula: width FOV (microns) = 167.789 * Voltage
conv_fact = 167.789
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

conv_fact should not be declared here. Add it to a ParametrizedQt class here or in the state and set the attribute gui=False

Comment thread twop/gui.py
)

def toggle_start(self):
if self.chk_drift_corr.isChecked() is True:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is True is redundant as the if statement is already evaluating isChecked() which returns only True or False

Comment thread twop/scanning.py
self.duration_queue = duration_queue
self.n_frames_queue = Queue()
self.correction_event = correction
self.correction_status = False
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why having a boolean and an event when you can just have the event?

Comment thread twop/scanning.py
self.setup_tasks(read_task, write_task, shutter_task)
if self.scanning_parameters.reset_shutter or toggle_shutter:
self.toggle_shutter(shutter_task)
pass
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is this?

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.

Drift correction

3 participants