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

Formalizing Featurizer interfaces #238

Merged
merged 36 commits into from Jun 12, 2019
Merged

Formalizing Featurizer interfaces #238

merged 36 commits into from Jun 12, 2019

Conversation

brimoor
Copy link
Contributor

@brimoor brimoor commented Jun 11, 2019

This PR addresses #235.

Change log

  • Adding ImageFeaturizer, VideoFramesFeaturizer, and VideoFeaturizer interfaces to eta.core.features that encapsulate the various image/tensor/video contracts that featurizers can use
  • replacing the old eta.core.features.VideoFramesFeaturizer with a new eta.core.features.CachingVideoFeaturizer class. CachingVideoFeaturizer has the same basic functionality as the former VideoFramesFeaturizer class with the addition of improved functionality for accessing the features on disk. Also formalizes the BackingManager interface
  • updating all existing featurizers in ETA to use the appropriate ImageFeaturizer, VideoFramesFeaturizer, and VideoFeaturizer interfaces
  • updating the video embedding examples in examples/demo_embed_vgg16 to use the new CachingVideoFeaturizer class

brimoor added 30 commits May 31, 2019 11:16
@brimoor brimoor added the feature Work on a feature request label Jun 11, 2019
@brimoor brimoor self-assigned this Jun 11, 2019
@brimoor brimoor changed the title Formalizing Featurizer interraces Formalizing Featurizer interfaces Jun 11, 2019
Copy link
Contributor

@mattphotonman mattphotonman left a comment

Choose a reason for hiding this comment

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

LGTM. Left a few minor comments, mostly about doc strings.

eta/core/features.py Outdated Show resolved Hide resolved
eta/core/features.py Outdated Show resolved Hide resolved
'''Configuration settings for an ORBFeaturizer.'''
def get_feature_paths(self):
'''Returns a list of absolute paths to the features on disk.'''
return etau.list_files(self.backing_dir, abs_paths=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there are other files in the backing directory besides the feature directories?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I added in a basic .endswith(".npz") filter.

As a general principle, we should avoid putting more than one type of thing in directories that contain sequences like %06.<ext>. For example, eta.core.utils.parse_dir_pattern is used heavily throughout the pipeline system, and it will get confused if there are multiple types of sequences in there.

eta/core/features.py Outdated Show resolved Hide resolved
@brimoor
Copy link
Contributor Author

brimoor commented Jun 12, 2019

@mattphotonman thanks for the careful review!

@brimoor brimoor merged commit 49ffc1c into develop Jun 12, 2019
@brimoor brimoor deleted the featurizer-tlc branch June 12, 2019 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Work on a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants