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

Ticket/425/motion/correction/tiffs #433

Merged
merged 46 commits into from
Feb 8, 2022
Merged

Ticket/425/motion/correction/tiffs #433

Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
8f978c6
_video_worker accepts a callable spatial_filter
danielsf Feb 3, 2022
ce64e54
add method to apply mean filter to videos
danielsf Feb 3, 2022
b65b335
validity check actually checks if all chunks were processed
danielsf Feb 3, 2022
246e454
video generation functions pass spatial_filter around
danielsf Feb 3, 2022
063511f
pass min/max into _video_array_from_h5 as kwargs
danielsf Feb 3, 2022
9ff2e8d
_video_array_from_h5 can cast to np.uint16
danielsf Feb 3, 2022
74942dc
add unit test to check exception in _video_array_from_h5
danielsf Feb 3, 2022
3ecc224
prepare _write_array_to_video for TIFF
danielsf Feb 3, 2022
3592c14
actually specify dtype in _video_array_from_h5
danielsf Feb 3, 2022
72c0ef4
can specify video_dtype in create_downsampled_video methods
danielsf Feb 3, 2022
793b13d
enable different filters and dtypes in CLI
danielsf Feb 3, 2022
e7f0412
get rid of errant np.uint8 hard codings
danielsf Feb 3, 2022
4124482
only save TIFFs as grayscale (otherwise, they are too big)
danielsf Feb 3, 2022
4063d94
pep8 changes
danielsf Feb 3, 2022
3dd4f34
fix invocation of Callable
danielsf Feb 3, 2022
fd0c102
unittest verifies that TIFFs are written as specified dtype
danielsf Feb 3, 2022
dc4fb7c
use suffix when determining how to write video
danielsf Feb 3, 2022
6811e7d
improve docstring for _write_array_to_video
danielsf Feb 3, 2022
6befbb9
fix error in docstring for apply_mean_filter_to_video
danielsf Feb 3, 2022
2f58098
fix error in docstring
danielsf Feb 3, 2022
1fd0600
add test to exercise error in create_side_by_side_video
danielsf Feb 3, 2022
dcd7321
add tests to make sure video output type is validated
danielsf Feb 4, 2022
ad3541a
fix description of kernel_size in schema
danielsf Feb 4, 2022
6664846
test different values of kernel_size
danielsf Feb 4, 2022
b955518
if quantiles not specified, set (0, 1)
danielsf Feb 4, 2022
5ce63c4
test handling of quantiles by video module
danielsf Feb 4, 2022
b5ccebb
remove reundante declaration of video_array
danielsf Feb 7, 2022
568608b
factor out code to create video array of uints from an HDF5 file
danielsf Feb 7, 2022
bafd616
factor out method to get supplemental args for video generation modules
danielsf Feb 7, 2022
312141b
rename video_path -> output_path in create_donwsampled_video
danielsf Feb 7, 2022
293e5c4
programmatically find max/half values for dtype
danielsf Feb 7, 2022
f144278
oops
danielsf Feb 7, 2022
ae549dc
factor out add_reticle
danielsf Feb 8, 2022
40b1555
rename apply_mean_filter_to_video -> apply_downsampled_mean_filter_to…
danielsf Feb 8, 2022
256287b
explicitly identify ncols for a single video in side_by_side method
danielsf Feb 8, 2022
ca9010e
factor out method to get frame size after spatial filtering
danielsf Feb 8, 2022
8ba18d0
use numpy to programmatically set dtype
danielsf Feb 8, 2022
8ae5f3c
_write_array_to_video can handle grayscale videos
danielsf Feb 8, 2022
8fa8a40
remove errant print statement
danielsf Feb 8, 2022
8ea25d6
add specificity to apply_downsampled_mean_filter_to_video docstring
danielsf Feb 8, 2022
e56b121
flesh out kernel_type docstring in schema
danielsf Feb 8, 2022
26e709a
get rid of quantiles=None logic; just set default quantiles=(0, 1)
danielsf Feb 8, 2022
f0ef500
pare down unit tests (the run time was getting a little excessive)
danielsf Feb 8, 2022
67df7ee
clarify unit test purposes
danielsf Feb 8, 2022
bf6523c
pare down combinations in end-to-end video unit test
danielsf Feb 8, 2022
534a109
rename util_classes -> cli_mixins
danielsf Feb 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions src/ophys_etl/modules/video/schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import argschema
import pathlib
from marshmallow import post_load
from marshmallow.validate import OneOf


class VideoBaseSchema(argschema.ArgSchema):
Expand All @@ -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; "
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...)

"if None or zero, no spatial filter applied"))

kernel_type = argschema.fields.String(
required=False,
allow_none=False,
default='median',
validation=OneOf(('median', 'mean')),
description=("Type of spatial smoothing kernel to be "
Copy link
Contributor

@aamster aamster Feb 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"applied to the video after downsampling"))

video_dtype = argschema.fields.String(
required=False,
allow_none=False,
default='uint8',
validation=OneOf(('uint8', 'uint16')),
description=("Type to which the output video is cast"))

input_frame_rate_hz = argschema.fields.Float(
required=True,
Expand Down Expand Up @@ -113,8 +129,10 @@ def check_quality(self, data, **kwargs):
def check_output_path(self, data, **kwargs):
output_path = pathlib.Path(data['output_path'])
output_suffix = output_path.suffix
if output_suffix not in ('.mp4', '.avi'):
msg = "output_path must be an .mp4 or .avi file\n"
allowed = ('.mp4', '.avi', '.tiff', '.tif')
if output_suffix not in allowed:
msg = "output_path must have one of these extensions:\n"
msg += f"{allowed}\n"
msg += f"you gave {data['output_path']}"
raise ValueError(msg)
return data
35 changes: 31 additions & 4 deletions src/ophys_etl/modules/video/side_by_side_video.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
from ophys_etl.modules.median_filtered_max_projection.utils import (
apply_median_filter_to_video)

from ophys_etl.modules.video.utils import (
create_side_by_side_video)
create_side_by_side_video,
apply_mean_filter_to_video)

from ophys_etl.modules.video.schemas import (
VideoBaseSchema)

from functools import partial
import numpy as np
import argschema
import pathlib

Expand Down Expand Up @@ -34,21 +40,42 @@ def run(self):
quantiles = (self.args['lower_quantile'],
self.args['upper_quantile'])
else:
quantiles = None
quantiles = (0.0, 1.0)

use_kernel = False
if self.args['kernel_size'] is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a new argument use_spatial_filter will simplify this logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if self.args['kernel_size'] > 0:
use_kernel = True

if use_kernel:
if self.args['kernel_type'] == 'median':
spatial_filter = partial(apply_median_filter_to_video,
kernel_size=self.args['kernel_size'])
else:
spatial_filter = partial(apply_mean_filter_to_video,
kernel_size=self.args['kernel_size'])
else:
spatial_filter = None

if self.args['video_dtype'] == 'uint8':
video_dtype = np.uint8
else:
video_dtype = np.uint16

create_side_by_side_video(
pathlib.Path(self.args['left_video_path']),
pathlib.Path(self.args['right_video_path']),
self.args['input_frame_rate_hz'],
pathlib.Path(self.args['output_path']),
self.args['output_frame_rate_hz'],
self.args['kernel_size'],
spatial_filter,
self.args['n_parallel_workers'],
quality=self.args['quality'],
quantiles=quantiles,
reticle=self.args['reticle'],
speed_up_factor=self.args['speed_up_factor'],
tmp_dir=self.args['tmp_dir'])
tmp_dir=self.args['tmp_dir'],
video_dtype=video_dtype)


if __name__ == "__main__":
Expand Down
35 changes: 31 additions & 4 deletions src/ophys_etl/modules/video/single_video.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
from ophys_etl.modules.median_filtered_max_projection.utils import (
apply_median_filter_to_video)

from ophys_etl.modules.video.utils import (
create_downsampled_video)
create_downsampled_video,
apply_mean_filter_to_video)

from ophys_etl.modules.video.schemas import (
VideoBaseSchema)

from functools import partial
import numpy as np
import argschema
import pathlib

Expand All @@ -26,20 +32,41 @@ def run(self):
quantiles = (self.args['lower_quantile'],
self.args['upper_quantile'])
else:
quantiles = None
quantiles = (0.0, 1.0)

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

if use_kernel:
if self.args['kernel_type'] == 'median':
spatial_filter = partial(apply_median_filter_to_video,
kernel_size=self.args['kernel_size'])
else:
spatial_filter = partial(apply_mean_filter_to_video,
kernel_size=self.args['kernel_size'])
else:
spatial_filter = None

if self.args['video_dtype'] == 'uint8':
danielsf marked this conversation as resolved.
Show resolved Hide resolved
video_dtype = np.uint8
else:
video_dtype = np.uint16

create_downsampled_video(
pathlib.Path(self.args['video_path']),
self.args['input_frame_rate_hz'],
pathlib.Path(self.args['output_path']),
self.args['output_frame_rate_hz'],
self.args['kernel_size'],
spatial_filter,
self.args['n_parallel_workers'],
quality=self.args['quality'],
quantiles=quantiles,
reticle=self.args['reticle'],
speed_up_factor=self.args['speed_up_factor'],
tmp_dir=self.args['tmp_dir'])
tmp_dir=self.args['tmp_dir'],
video_dtype=video_dtype)


if __name__ == "__main__":
Expand Down
Loading