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

Rep frame helper #278

Merged
merged 7 commits into from
Jun 6, 2024
Merged

Rep frame helper #278

merged 7 commits into from
Jun 6, 2024

Conversation

kelleyl
Copy link
Contributor

@kelleyl kelleyl commented May 28, 2024

Adds two functions to video_document_helper.py. Functions are used to extract the image for the representative timepoint for a given timeframe annotation.

Copy link

codecov bot commented May 28, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 91.41%. Comparing base (262015b) to head (fa069fc).

Files Patch % Lines
mmif/utils/video_document_helper.py 83.33% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #278      +/-   ##
===========================================
- Coverage    91.54%   91.41%   -0.13%     
===========================================
  Files           10       10              
  Lines         1265     1282      +17     
===========================================
+ Hits          1158     1172      +14     
- Misses         107      110       +3     
Flag Coverage Δ
unittests 91.41% <83.33%> (-0.13%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@keighrim keighrim left a comment

Choose a reason for hiding this comment

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

looks good! I left a small suggestion to use long_id and Mmif.get() for retrieving annotation objects.

Comment on lines 146 to 152
representative_timepoint_anno = None
for view in views:
try:
representative_timepoint_anno = view.get_annotation_by_id(top_representative_id)
break
except KeyError:
continue
Copy link
Member

Choose a reason for hiding this comment

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

this can be much shorter if using

def __getitem__(self, item: str) -> Union[Document, View, Annotation]:
"""
getitem implementation for Mmif. When nothing is found, this will raise an error
rather than returning a None (although pytype doesn't think so...)
:raises KeyError: if the item is not found or if the search results are ambiguous
:param item: the search string, a document ID, a view ID, or a view-scoped annotation ID
:return: the object searched for
"""
if item in self._named_attributes():
return self.__dict__[item]
split_attempt = item.split(self.id_delimiter)

and
@property
def long_id(self) -> str:
if self.parent is not None and len(self.parent) > 0:
return f"{self.parent}{self.id_delimiter}{self.id}"
else:
return self.id

video_document = mmif[time_frame.get_property('document')]
fps = get_framerate(video_document)
representatives = time_frame.get_property('representatives')
top_representative_id = representatives[0]
Copy link
Member

Choose a reason for hiding this comment

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

can we provide an argument to set the rule to pick the "top"? I'm thinking of

  1. get all fnums from all elements in the rep list.
  2. get the fnum of the first rep element (as a single list maybe, only to keep the return type simple)
  3. get the fnum of the "middle" rep element

We might need to think more carefully how this argument can be propagated in the following extract_representative_frame wrapper fn.

@keighrim keighrim merged commit 6cec53f into develop Jun 6, 2024
1 of 3 checks passed
@keighrim keighrim deleted the rep-frame-helper branch June 6, 2024 23:02
@keighrim keighrim mentioned this pull request Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants