Ticket/425/motion/correction/tiffs #433

danielsf · 2022-02-03T22:21:44Z

During the 2/2/2022 meeting with the SSF team, the scientists requested that we produce spatiotemporally downsampled TIFF files to support their QC of motion correction. This PR modifies our existing ophys_etl/modules/video modules to produce those files. Examples of the output can be found in

/allen/programs/mindscope/workgroups/surround/motion_correction_labeling_2022/**/*tiff

Jun Zhuang has looked at examples of these files and is happy with the output.

this will allow us to more easily swap in something other than a median_filter on demand

(not kernel_size)

TIFFs are limited to 4GB in size because of 32 bit offsets https://stackoverflow.com/questions/62245475/how-to-save-a-very-large-numpy-array-as-an-image-loading-as-little-as-possible

codecov · 2022-02-03T23:12:33Z

Codecov Report

Merging #433 (534a109) into main (5159c07) will increase coverage by 0.34%.
The diff coverage is 98.33%.

@@            Coverage Diff             @@
##             main     #433      +/-   ##
==========================================
+ Coverage   90.23%   90.57%   +0.34%     
==========================================
  Files          88       89       +1     
  Lines        5426     5456      +30     
==========================================
+ Hits         4896     4942      +46     
+ Misses        530      514      -16

Flag	Coverage Δ
not_container_tests	`90.57% <98.33%> (+0.34%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/ophys_etl/modules/video/utils.py	`97.29% <97.77%> (+1.56%)`	⬆️
src/ophys_etl/modules/video/cli_mixins.py	`100.00% <100.00%> (ø)`
src/ophys_etl/modules/video/schemas.py	`97.56% <100.00%> (+25.22%)`	⬆️
src/ophys_etl/modules/video/side_by_side_video.py	`87.50% <100.00%> (+5.14%)`	⬆️
src/ophys_etl/modules/video/single_video.py	`86.66% <100.00%> (+5.41%)`	⬆️

morriscb

Found one thing that I have questions about. Also, there seems to be a lot of duplicated code in this module/ticket. Is there a way of possibly eliminating some of the duplicated code?

src/ophys_etl/modules/video/utils.py

aamster · 2022-02-07T21:05:46Z

src/ophys_etl/modules/video/schemas.py

@@ -16,8 +17,23 @@ class VideoBaseSchema(argschema.ArgSchema):
            required=False,
            allow_none=True,
            default=3,
-            description=("Radius of median filter kernel; "
-                         "if None, no median filter applied"))
+            description=("Size of spatial filter kernel; "


Can you instead add a new argument use_spatial_filter to control whether to apply spatial filtering? Passing None or 0 here is a little awkward, imo.

I am of the opposite opinion. I don't like the idea of adding more parameters to the schema. It clutters up --help and leaves more places for users to screw up (setting use_spatial_filter=False, kernel_size=2 when they intended to use a kernel of size 2). I like the idea that you can just turn filtering off by setting the kernel size to nothing. We can talk about this, if you want.

In that case you could just issue a warning that use_spatial_filter is False but kernel_size is not None

To me, that seems just as complicated as the present

use_kernel = False if self.args['kernel_size'] is not None: if self.args['kernel_size'] > 0: use_kernel = True

I'm going to leave this.

aamster · 2022-02-07T21:06:22Z

src/ophys_etl/modules/video/side_by_side_video.py

+            quantiles = (0.0, 1.0)
+
+        use_kernel = False
+        if self.args['kernel_size'] is not None:


Adding a new argument use_spatial_filter will simplify this logic

I know, I just think the simplicity in the CLI is worth it.

aamster · 2022-02-07T21:15:29Z

src/ophys_etl/modules/video/utils.py

@@ -30,13 +28,14 @@ def create_downsampled_video(
        input_hz: float,
        video_path: pathlib.Path,
        output_hz: float,
-        kernel_size: Optional[int],
+        spatial_filter: Optional[Callable[[np.ndarray], np.ndarray]],


Can create_downsampled_video_h5 instead convert the spatial_filter argument as a string into a function rather than the CLI constructing it and passing a function around?

If we wanted to use this without going through the CLI, we would have to replicate that logic.

I actually like that these methods accept callables. It means that, outside of the CLI, we can quickly experiment with new spatial filters by just passing in a function without having to implement the if/else block necessary to parse a string into a function.

Again: open to discussion, but I do see this as a feature, rather than a bug.

Sure, then you allow this function to accept string or callable. If string, it converts it into a callable.
This would not force the user to go through the CLI to be able to properly pull the correct function for "mean" or "median" filters and would allow your use case of passing a custom callable for experimenting.

I'm going to leave this. Your proposal would require adding a kernel_size arg to create_downsampled_video_h5. I'm not a huge fan of increasing the number of arguments more than we have to. When/if we move to class-based video manipulation, I suspect the apply_spatial_filter() method will be general enough to accept an object that is a spatial filter (I know I'm talking like someone who lives in C++-ville; in python, it would just be a callable). I still like the generalizability of that. We can revisit this conversation when we move to class-based manipulation of videos.

src/ophys_etl/modules/video/utils.py

aamster · 2022-02-07T22:16:51Z

src/ophys_etl/modules/video/utils.py

+    """
+
+    downsampled_video = skimage_measure.block_reduce(
+                            video,


It's a bummer we can't combine our median filter with mean filter somehow. They are both doing the same thing except they use different functions and do/don't do downsampling

Probably the responsible thing to do would be to consolidate these in a video_utils module. Do you mind if we leave that work fro #436 (if you agree, I'll add this as an explicit bullet to that ticket)?

src/ophys_etl/modules/video/single_video.py

…_video

danielsf · 2022-02-08T01:24:39Z

@aamster @morriscb I think this is ready for a re-review.

aamster

Looks good, left some nitpicks.

aamster · 2022-02-08T13:50:28Z

src/ophys_etl/modules/video/schemas.py

@@ -16,8 +17,23 @@ class VideoBaseSchema(argschema.ArgSchema):
            required=False,
            allow_none=True,
            default=3,
-            description=("Radius of median filter kernel; "
-                         "if None, no median filter applied"))
+            description=("Size of spatial filter kernel; "


Sorry can you please make it clear that these kernels apply specifically downsampling . It is possible to apply a sliding kernel with no downsampling

Our default median filter actually doesn't apply downsampling.

https://github.com/AllenInstitute/ophys_etl_pipelines/blob/main/src/ophys_etl/modules/median_filtered_max_projection/utils.py#L12-L41

The size of the frame coming in is the size of the frame coming out

>>> import numpy as np >>> import scipy.ndimage as scipy_ndimage >>> f = np.random.random((25, 25)) >>> >>> f2 = scipy_ndimage.median_filter(f, size=3, mode='reflect') >>> f.shape (25, 25) >>> f2.shape (25, 25)

(because this isn't complicated enough...)

aamster · 2022-02-08T13:51:04Z

src/ophys_etl/modules/video/schemas.py

+            allow_none=False,
+            default='median',
+            validation=OneOf(('median', 'mean')),
+            description=("Type of spatial smoothing kernel to be "


Same thoughts above about mentioning "downsampling". Also I'm assuming the "downsampling" mentioned already here is the temporal downsampling

I modified the docstring to explicitly call out that the "mean" filter downsamples the video frames while the "median" filter does not.

aamster · 2022-02-08T14:08:38Z

src/ophys_etl/modules/video/util_classes.py

+            quantiles = (self.args['lower_quantile'],
+                         self.args['upper_quantile'])
+        else:
+            quantiles = (0.0, 1.0)


Could you instead change the default to 0 and 1 instead of None? Then this logic wouldn't be needed.

aamster · 2022-02-08T14:10:40Z

src/ophys_etl/modules/video/schemas.py

@@ -16,8 +17,23 @@ class VideoBaseSchema(argschema.ArgSchema):
            required=False,
            allow_none=True,
            default=3,
-            description=("Radius of median filter kernel; "
-                         "if None, no median filter applied"))
+            description=("Size of spatial filter kernel; "


In that case you could just issue a warning that use_spatial_filter is False but kernel_size is not None

aamster · 2022-02-08T14:12:09Z

src/ophys_etl/modules/video/util_classes.py

@@ -0,0 +1,44 @@
+from ophys_etl.modules.video.utils import (


Could you please make it clear that this is specific to the CLI? Ie by calling this module cli_mixin or by placing it in a cli package

aamster · 2022-02-08T14:14:31Z

src/ophys_etl/modules/video/utils.py

@@ -30,13 +28,14 @@ def create_downsampled_video(
        input_hz: float,
        video_path: pathlib.Path,
        output_hz: float,
-        kernel_size: Optional[int],
+        spatial_filter: Optional[Callable[[np.ndarray], np.ndarray]],


Sure, then you allow this function to accept string or callable. If string, it converts it into a callable.
This would not force the user to go through the CLI to be able to properly pull the correct function for "mean" or "median" filters and would allow your use case of passing a custom callable for experimenting.

aamster · 2022-02-08T14:26:52Z

src/ophys_etl/modules/video/utils.py

-    kernel_size: Optional[int]
-        Size of the median filter kernel to be applied to the downsampled
-        movie (if None, no median filter will be applied)
+    spatial_filter: Optional[Callable[[np.ndarray], np.ndarray]]


My thinking is this -- if what you are modeling can be thought of as a thing, then it should use a class-based design. Video is a thing, therefore it should use a class-based design. I don't think we necessarily need to carry around the whole video in memory with this approach.

src/ophys_etl/modules/video/utils.py

aamster · 2022-02-08T14:32:57Z

src/ophys_etl/modules/video/utils.py

                                dtype=float)

    mgr = multiprocessing.Manager()
    output_lock = mgr.Lock()
    validity_dict = mgr.dict()
    process_list = []
+
+    print(f'spatial filter {spatial_filter}')


Maybe you meant to use the logger?

...I think I actually meant to remove that print statement. I added that when I was implementing spatial_filter to make sure it got passed through. Thanks for catching that.

run time was getting excessive

danielsf added 19 commits February 2, 2022 16:47

_video_worker accepts a callable spatial_filter

8f978c6

this will allow us to more easily swap in something other than a median_filter on demand

add method to apply mean filter to videos

ce64e54

validity check actually checks if all chunks were processed

b65b335

video generation functions pass spatial_filter around

246e454

(not kernel_size)

pass min/max into _video_array_from_h5 as kwargs

063511f

_video_array_from_h5 can cast to np.uint16

9ff2e8d

add unit test to check exception in _video_array_from_h5

74942dc

prepare _write_array_to_video for TIFF

3ecc224

actually specify dtype in _video_array_from_h5

3592c14

can specify video_dtype in create_downsampled_video methods

72c0ef4

enable different filters and dtypes in CLI

793b13d

get rid of errant np.uint8 hard codings

e7f0412

only save TIFFs as grayscale (otherwise, they are too big)

4124482

TIFFs are limited to 4GB in size because of 32 bit offsets https://stackoverflow.com/questions/62245475/how-to-save-a-very-large-numpy-array-as-an-image-loading-as-little-as-possible

pep8 changes

4063d94

fix invocation of Callable

3dd4f34

unittest verifies that TIFFs are written as specified dtype

fd0c102

use suffix when determining how to write video

dc4fb7c

improve docstring for _write_array_to_video

6811e7d

fix error in docstring for apply_mean_filter_to_video

6befbb9

danielsf added 7 commits February 3, 2022 15:56

fix error in docstring

2f58098

add test to exercise error in create_side_by_side_video

1fd0600

add tests to make sure video output type is validated

dcd7321

fix description of kernel_size in schema

ad3541a

test different values of kernel_size

6664846

if quantiles not specified, set (0, 1)

b955518

test handling of quantiles by video module

5ce63c4

danielsf marked this pull request as ready for review February 4, 2022 00:25

morriscb reviewed Feb 7, 2022

View reviewed changes

src/ophys_etl/modules/video/utils.py Outdated Show resolved Hide resolved

danielsf mentioned this pull request Feb 7, 2022

Ticket/429/artifact/creation #435

Merged

This was referenced Feb 7, 2022

Consolidate video generation code #436

Open

Collect requirements for downsampled movies coming off of motion correction #412

Open

danielsf added 2 commits February 7, 2022 13:38

remove reundante declaration of video_array

b5ccebb

factor out code to create video array of uints from an HDF5 file

568608b

aamster reviewed Feb 7, 2022

View reviewed changes

danielsf added 10 commits February 7, 2022 15:12

factor out method to get supplemental args for video generation modules

bafd616

rename video_path -> output_path in create_donwsampled_video

312141b

programmatically find max/half values for dtype

293e5c4

oops

f144278

factor out add_reticle

ae549dc

rename apply_mean_filter_to_video -> apply_downsampled_mean_filter_to…

40b1555

…_video

explicitly identify ncols for a single video in side_by_side method

256287b

factor out method to get frame size after spatial filtering

ca9010e

use numpy to programmatically set dtype

8ba18d0

_write_array_to_video can handle grayscale videos

8ae5f3c

aamster approved these changes Feb 8, 2022

View reviewed changes

danielsf added 8 commits February 8, 2022 10:20

remove errant print statement

8fa8a40

add specificity to apply_downsampled_mean_filter_to_video docstring

8ea25d6

flesh out kernel_type docstring in schema

e56b121

get rid of quantiles=None logic; just set default quantiles=(0, 1)

26e709a

pare down unit tests (the run time was getting a little excessive)

f0ef500

clarify unit test purposes

67df7ee

pare down combinations in end-to-end video unit test

bf6523c

run time was getting excessive

rename util_classes -> cli_mixins

534a109

danielsf force-pushed the ticket/425/motion/correction/tiffs branch from bd5a33e to 534a109 Compare February 8, 2022 19:56

danielsf merged commit 8ce066a into main Feb 8, 2022

This was referenced Feb 8, 2022

downsampled motion corrected movie AllenInstitute/brain_observatory_qc#16

Closed

Design a class for the manipulation of Ophys videos in ophys_etl_pipelines #438

Open

danielsf deleted the ticket/425/motion/correction/tiffs branch February 17, 2022 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ticket/425/motion/correction/tiffs #433

Ticket/425/motion/correction/tiffs #433

danielsf commented Feb 3, 2022 •

edited

Loading

codecov bot commented Feb 3, 2022 •

edited

Loading

morriscb left a comment

aamster Feb 7, 2022

danielsf Feb 8, 2022

aamster Feb 8, 2022

danielsf Feb 8, 2022

aamster Feb 7, 2022

danielsf Feb 8, 2022

aamster Feb 7, 2022 •

edited

Loading

danielsf Feb 8, 2022

aamster Feb 8, 2022

danielsf Feb 8, 2022

aamster Feb 7, 2022

danielsf Feb 8, 2022

danielsf commented Feb 8, 2022

aamster left a comment

aamster Feb 8, 2022

danielsf Feb 8, 2022

aamster Feb 8, 2022 •

edited

Loading

danielsf Feb 8, 2022

aamster Feb 8, 2022

danielsf Feb 8, 2022

aamster Feb 8, 2022

aamster Feb 8, 2022

danielsf Feb 8, 2022

aamster Feb 8, 2022

aamster Feb 8, 2022

aamster Feb 8, 2022

danielsf Feb 8, 2022

Ticket/425/motion/correction/tiffs #433

Ticket/425/motion/correction/tiffs #433

Conversation

danielsf commented Feb 3, 2022 • edited Loading

codecov bot commented Feb 3, 2022 • edited Loading

Codecov Report

morriscb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aamster Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielsf commented Feb 8, 2022

aamster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aamster Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielsf commented Feb 3, 2022 •

edited

Loading

codecov bot commented Feb 3, 2022 •

edited

Loading

aamster Feb 7, 2022 •

edited

Loading

aamster Feb 8, 2022 •

edited

Loading