Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Optical flow dataset loader #12

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

Conversation

fral92
Copy link
Contributor

@fral92 fral92 commented Jun 5, 2017

  • The optical flow is now loaded directly from the parallel loader
  • This makes the OF loading no dataset-specific
  • Add parameters to decide if the OF has to be computed or loaded from path
  • Add parameter to select the OF type

@@ -365,6 +372,10 @@ def random_transform(x, y=None,
An image.
y: array of int
An array with labels.
sequence_names: list of strings
A list of prefix and names for the current sequence
Copy link
Owner

Choose a reason for hiding this comment

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

What does it mean?

sequence_names: list of strings
A list of prefix and names for the current sequence
data_path: string
Current dataset path
Copy link
Owner

Choose a reason for hiding this comment

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

You always pass self.path here:

  1. call it path
  2. change the docstring into: "The local path of the dataset. Hardcoded to self.path"

if return_optical_flow:
from skimage import io
flow = []
if compute_optical_flow:
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than having yet another flag, can you check if the optical flow exists on disk and compute it (and save it) only if it's missing?

Copy link
Owner

Choose a reason for hiding this comment

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

Note: add a shared_path parameter as well so that if the OF is missing in the local path you can look for it in the shared path. If it's missing in both path you compute it and save it in both paths.

flow = []
if compute_optical_flow:
flow = optical_flow(x, rows_idx, cols_idx, chan_idx,
return_rgb=return_optical_flow == 'rgb')
Copy link
Owner

Choose a reason for hiding this comment

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

Why is optical_flow_type not being used here? I think it should.

return_rgb=return_optical_flow == 'rgb')
else:
if optical_flow_type not in ['Brox', 'Farn', 'LK', 'TVL1']:
raise RuntimeError('Unknown optical flow type')
Copy link
Owner

Choose a reason for hiding this comment

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

As I said, I think optical_flow_type should be used also in the other branch. Therefore this check should be done at the beginning of this new section (L588)

flow = optical_flow(x, rows_idx, cols_idx, chan_idx,
return_rgb=return_optical_flow=='rgb')
x = np.concatenate((x, flow), axis=chan_idx)
x = np.concatenate((x, np.array(flow)), axis=chan_idx)
Copy link
Owner

Choose a reason for hiding this comment

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

ditto (move before)

optical_flow_type: string
Indicates the method used to generate the optical flow. The
optical flow is loaded from a specific directory based on this
type.
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure what all of these mean, but I'll comment on the docstrings once you fix the code.

@@ -562,6 +585,43 @@ def random_transform(x, y=None,
fill_mode=fill_mode,
fill_constant=cvalMask,
rows_idx=rows_idx, cols_idx=cols_idx))
flow = None
Copy link
Owner

Choose a reason for hiding this comment

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

No need for this (see suggested modifications below)

of = of.astype(x.dtype) / 255.
else:
raise RuntimeError('Optical flow not found for this '
'file: %s' % of_path)
Copy link
Owner

Choose a reason for hiding this comment

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

I am having a really hard time understanding this code. can you please answer these questions when you have time?

  1. What does sequence_names stand for?
  2. This is my understanding of L606-622. Can you confirm it is correct and intended?
    for _ in sequence_names:
        if file_index == 0 and not repeat_1st_opt_flow:
            of = np.zeros(x.shape[1:], x.dtype)
        else:
            if repeat_1st_opt_flow:
                frame = filenames[0]
            else:
                frame = filenames[-1]
            of_path = os.path.join(optical_flow_path, frame + 'jpg')
            of = io.imread(of_path).astype(x.dtype) / 255.
  3. If what I wrote is correct, why do you set the first OF to zero when repeat_1st_opt_flow is False?
  4. If what I wrote is correct, why do you return the OF of the last frame for every frame after the first, when repeat_1st_opt_flow is False?

self.path + '/')[1].split('/', 1)[1]
# Get all the filenames for the current batch to load
current_filenames = [fname for fname in self.filenames if
el[0][0] in fname]
Copy link
Owner

Choose a reason for hiding this comment

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

I am not sure if this is right. My understanding is that it's filtering the filenames with the same prefix as the one of the current batch. Is that right? If so, do you really need to do it for each batch? It seems expensive.

If this makes sense, since we would end up passing self.path, self.shared_path and current_filenames which is also computed out of self, it's probably more convenient to pass self directly to the random_transform function (e.g., as a dataset argument) and access all those attributes from the function itself.

* Do not create pointer seq_x, seq_y. It is easy to introduce bugs when operations on them are not reflected in the original dictioary.
* Pass the dataset object rather than all its parameteres.
* NOTE: This commit breaks the optical flow. Will be fixed in the next commit.
@fvisin fvisin force-pushed the optical_flow branch 2 times, most recently from ed8f880 to ca55ff1 Compare October 26, 2017 21:22
fvisin and others added 4 commits October 27, 2017 17:57
* Allow to load OF from disk from .npy files
* Compute the OF at run time if missing (only Farneback available ATM)
* Add parameter to select the OF type
* Add parameter to select whether to return OF as RGB or displacement
* Better OF visualization, adapted from TransFlow
@fvisin
Copy link
Owner

fvisin commented Nov 17, 2017

@marcociccone can you review this when you have time?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants