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

Change how time points are associated with bounding boxes #1

Closed
kelleyl opened this issue Nov 24, 2020 · 6 comments
Closed

Change how time points are associated with bounding boxes #1

kelleyl opened this issue Nov 24, 2020 · 6 comments

Comments

@kelleyl
Copy link
Contributor

kelleyl commented Nov 24, 2020

Instead of generating an alignment annotation, add the id to bounding box annotation properties.

@keighrim
Copy link
Member

I wonder if this is actually what we want. We might need to think an talk through how we want to handle alignment between image regions and time spans / points. Bringing @marcverhagen in to the dicsussion.

@marcverhagen
Copy link

With alignment I am constantly hopping from one approach to another: (1) an alignment as a full-blown relations between annotations, with all the bells and whistles that we may need, and (2) alignment as a property on one annotation that points to another. The second is simpler, but may be too simple.

Related to that we have had some discussions on what anchors do and I think the two intersect.

@clams-bot clams-bot added this to apps Apr 23, 2023
@keighrim keighrim moved this to Todo in apps Apr 23, 2023
@keighrim
Copy link
Member

keighrim commented Jun 19, 2023

While working on #4 (and #5), I found the proposal here was already the case (not generating TimePoint annotations) except the timePoints property were not actually used, instead frame property was used to record the frame number.
(see 5321c75#diff-7a283e35179cfc16f178c2af0777e7e39ba628bbc49d30da0f814b627869c290R132-L140 )

So when #5 is merged, we can consider this issue is resolved. However, I wonder this "bugfix" of mine will effect mmif-viz and wanted to bring @haydenmccormick into the conversation.

Also for the full context, the code linked above was updated with full support for other timeunits in the following commits in #5.

if 'frame' in output_unit:
timepoint = frame_number
else:
timepoint = frame_number / cap.get(cv2.CAP_PROP_FPS)
if "millisecond" in output_unit:
timepoint = int(timepoint * 1000)
elif "second" in output_unit:
timepoint = round(timepoint, 2)
else:
raise ValueError(f"Invalid output time unit: {output_unit}")
bb_annotation.add_property("timePoint", timepoint)

@keighrim
Copy link
Member

keighrim commented Jun 19, 2023

The 'frames' property has also been used in downstream apps, for example tesseract-wrapper.

linking these PR as reminders to fix.

@keighrim
Copy link
Member

Given our recent discussion about setting a standard time unit (clamsproject/mmif#192), I think we need to re-consider this stop-gap implementation (having timePoint property in BoundingBox object), and instead move to using more standard way of (TimePoint, BoundingBox, Alignment) triple to constrain all time-related anchor objects to TimePoint and TimeFrame instances only.

@keighrim
Copy link
Member

fixed by #8.

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

No branches or pull requests

4 participants