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

Updated structure #13

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Conversation

jeromelecoq
Copy link

This PR follow up on your great Hackathonwork and introduces a few useful things:

  1. 2 calibrating classes that share the same interface to facilitate comparison
  2. A standardization of variable names throughout
  3. 2 notebooks to evaluate both compression approaches
  4. Tests that compare the calibration directly to make sure we are starting from the same place
  5. A modification of the encoding format to better match Zarr philosophy
  6. Simplification of the code throughout

Copy link
Member

@dimitri-yatsenko dimitri-yatsenko left a comment

Choose a reason for hiding this comment

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

I am marking these as a comment for now.

return decoded.astype(self.decoded_dtype)
if self.use_lookup:
# produce anscombe lookup_tables
input_max = np.iinfo(self.decoded_dtype).max
Copy link
Member

Choose a reason for hiding this comment

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

oof, what if it's int64?


# https://en.wikipedia.org/wiki/Anscombe_transform for the forward
Copy link
Member

@dimitri-yatsenko dimitri-yatsenko Mar 2, 2024

Choose a reason for hiding this comment

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

We subtract sqrt(⅜) to map zero in the input to zero in the output. Otherwise. 0 will become 1 when rounding.

@@ -4,12 +4,12 @@

This codec is designed for compressing movies with Poisson noise, which are produced by photon-limited modalities such multiphoton microscopy, radiography, and astronomy.

The codec assumes that the video is linearly encoded with a potential offset (`zero_level`) and that the `photon_sensitivity` (the average increase in intensity per photon) is known or can be accurately estimated from the data.
The codec assumes that the video is linearly encoded with a potential offset (`dark_signal`) and that the `photon_sensitivity` (the average increase in intensity per photon) is known or can be accurately estimated from the data.
Copy link
Member

@dimitri-yatsenko dimitri-yatsenko Mar 2, 2024

Choose a reason for hiding this comment

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

Why dark_signal? "Signal" implies a time series. I would use a term that indicate a scalar value, e.g. "dark_level" or "dark_value".

Comment on lines 353 to 354
slope = found_fits[i, 0]
offset = found_fits[i, 1]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
slope = found_fits[i, 0]
offset = found_fits[i, 1]
slope, offset = found_fits[i, :]

if self.use_lookup:
decoded = self._lookup(dec, self.inverse_table)
else:
# We first unapply beta
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# We first unapply beta

# We convert back to arbitrary pixels
dec = dec * self.photon_sensitivity + self.dark_signal

# We have to go back to integers
Copy link
Member

Choose a reason for hiding this comment

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

The code explains itself.

Suggested change
# We have to go back to integers
# We have to go back to integers

Comment on lines +55 to +61
# JEROME: I do not understand why we need to subtract np.sqrt(3/8) here
# Also, shouldn't te +3/8 be inside the maximum function as xx is a float?
forward = 2.0 / self.beta * (np.sqrt(np.maximum(0, xx) + 3/8) - np.sqrt(3/8))

# JEROME : I think this might be better syntax?
forward = np.round(forward).astype(self.encoded_dtype)
self.forward_table = forward
Copy link
Member

@dimitri-yatsenko dimitri-yatsenko Mar 2, 2024

Choose a reason for hiding this comment

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

We subtracted sqrt(⅜) to map zero in the input to zero in the output.

Suggested change
# JEROME: I do not understand why we need to subtract np.sqrt(3/8) here
# Also, shouldn't te +3/8 be inside the maximum function as xx is a float?
forward = 2.0 / self.beta * (np.sqrt(np.maximum(0, xx) + 3/8) - np.sqrt(3/8))
# JEROME : I think this might be better syntax?
forward = np.round(forward).astype(self.encoded_dtype)
self.forward_table = forward
forward = 2.0 / self.beta * (np.sqrt(np.maximum(0, xx) + 3/8) - np.sqrt(3/8))
self.forward_table = forward.astype(self.encoded_dtype)

As long as the reverse is computed correctly from this forward function, then it does not make a big difference.
We just want to use the range of values starting from zero.

Comment on lines +68 to +72
def _lookup(self, movie, LUT):
"""
Apply lookup table LUT to input movie
"""
return LUT[np.maximum(0, np.minimum(movie, LUT.size-1))]
Copy link
Member

@dimitri-yatsenko dimitri-yatsenko Mar 2, 2024

Choose a reason for hiding this comment

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

Suggested change
def _lookup(self, movie, LUT):
"""
Apply lookup table LUT to input movie
"""
return LUT[np.maximum(0, np.minimum(movie, LUT.size-1))]
def _lookup(self, movie, lookup_table):
"""
Apply lookup table to input movie
"""
return lookup_table[np.maximum(0, np.minimum(movie, lookup_table.size-1))]


return [self.photon_sensitivity, self.dark_signal]

def plot_poisson_curve(self):
Copy link
Member

@dimitri-yatsenko dimitri-yatsenko Mar 2, 2024

Choose a reason for hiding this comment

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

In the literature, this is known as the Photon Transfer Curve.


class RasterCalibratePhotons():
class CalibratePhotons():
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need these classes? All the functionality here can be implemented as pure functions with greater readability and maintainability. Class hierarchies require a lot of engineering and should be avoided when a function will do.

Copy link
Member

Choose a reason for hiding this comment

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

"calibrate" is not quite the right word. We calibrate a device. What we are doing here is more like estimate_photon_sensitivity.

self.fitted_pixels_mean = None
self.fitted_model = None

def _longest_run(self, bool_array: np.ndarray) -> slice:
Copy link
Member

Choose a reason for hiding this comment

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

This does not use self. This is a pure function. Does not need to be in a class. You could use @staticmethod but really we don't need these classes here.

Comment on lines +55 to +86
def subsample_and_crop_video(self, crop, start_frame=0, end_frame=-1):
"""Subsample and crop a video, cache results. Also functions as a data_pointer load.

Args:
crop: A tuple (px_y, px_x) specifying the number of pixels to remove
start_frame: The index of the first desired frame
end_frame: The index of the last desired frame

Returns:
The resultant array.
"""

# We first reset the saved data
self.mean_image = None
self.std_image = None
self.photon_sensitivity = None
self.dark_signal = None

_shape = self.data_array_movie.shape
px_y_start, px_x_start = crop
px_y_end = _shape[1] - px_y_start
px_x_end = _shape[2] - px_x_start

if start_frame == _shape[0] - 1 and (end_frame == -1 or end_frame == _shape[0]):
cropped_video = self.data_array_movie[
start_frame:_shape[0], px_y_start:px_y_end, px_x_start:px_x_end
]
else:
cropped_video = self.data_array_movie[
start_frame:end_frame, px_y_start:px_y_end, px_x_start:px_x_end
]
self.data_array_movie = cropped_video
Copy link
Member

Choose a reason for hiding this comment

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

Why does this method belong in this library?

Suggested change
def subsample_and_crop_video(self, crop, start_frame=0, end_frame=-1):
"""Subsample and crop a video, cache results. Also functions as a data_pointer load.
Args:
crop: A tuple (px_y, px_x) specifying the number of pixels to remove
start_frame: The index of the first desired frame
end_frame: The index of the last desired frame
Returns:
The resultant array.
"""
# We first reset the saved data
self.mean_image = None
self.std_image = None
self.photon_sensitivity = None
self.dark_signal = None
_shape = self.data_array_movie.shape
px_y_start, px_x_start = crop
px_y_end = _shape[1] - px_y_start
px_x_end = _shape[2] - px_x_start
if start_frame == _shape[0] - 1 and (end_frame == -1 or end_frame == _shape[0]):
cropped_video = self.data_array_movie[
start_frame:_shape[0], px_y_start:px_y_end, px_x_start:px_x_end
]
else:
cropped_video = self.data_array_movie[
start_frame:end_frame, px_y_start:px_y_end, px_x_start:px_x_end
]
self.data_array_movie = cropped_video

inverse.size * (inverse[-1] - inverse[-2])/2).astype(self.decoded_dtype)
self.inverse_table = inverse

def _lookup(self, movie, LUT):
Copy link
Member

Choose a reason for hiding this comment

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

This function does not need self.

if self.use_lookup:
encoded = self._lookup(buf, self.forward_table)
else:
# We convert to photons
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# We convert to photons
# convert to photons

Comment on lines +88 to +89

# We have to go to integers in a clean way
Copy link
Member

@dimitri-yatsenko dimitri-yatsenko Mar 2, 2024

Choose a reason for hiding this comment

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

Here the code explains itself better than the comment.

Suggested change
# We have to go to integers in a clean way
# We have to go to integers in a clean way

# https://en.wikipedia.org/wiki/Anscombe_transform for the inverse without bias
dec = dec**2 / 4.0 - 1/8

# We convert back to arbitrary pixels
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# We convert back to arbitrary pixels
# restore original grayscale

[photon_gain, photon_offset]=calibrator.get_photon_gain_parameters(perc_min=0, perc_max=100)
print(photon_gain, photon_offset)
[photon_sensitivity, dark_signal]=calibrator.get_photon_sensitivity_parameters(perc_min=0, perc_max=100)
print(photon_sensitivity, dark_signal)

# We check that the gain and offset are within X% of the true value
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# We check that the gain and offset are within X% of the true value
# check that the gain and offset are within X% of the true value

return photon_flux

def get_photon_gain_parameters(self, max_pixel_range=2**15, n_groups=1, perc_min=3, perc_max=90):
def get_photon_sensitivity_parameters(self, max_pixel_range=2**15, n_groups=1, perc_min=3, perc_max=90):
Copy link
Member

@dimitri-yatsenko dimitri-yatsenko Mar 2, 2024

Choose a reason for hiding this comment

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

max_pixel_range you probably mean "value", not "range". It should be 2**15-1 == 32767 == 0x7FFF. 2**15 overflows in int16.


def _lookup(self, movie, LUT):
"""
Apply lookup table LUT to input movie
Copy link
Member

@dimitri-yatsenko dimitri-yatsenko Mar 2, 2024

Choose a reason for hiding this comment

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

Suggested change
Apply lookup table LUT to input movie
Apply lookup table to movie

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