Skip to content

Initial subject tracking implementation#84

Merged
brosenberg42 merged 12 commits into
developfrom
feat/subject-tracking
Feb 10, 2025
Merged

Initial subject tracking implementation#84
brosenberg42 merged 12 commits into
developfrom
feat/subject-tracking

Conversation

@brosenberg42
Copy link
Copy Markdown
Member

@brosenberg42 brosenberg42 commented Jul 10, 2024

Copy link
Copy Markdown
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @brosenberg42)


subject/api/mpf_subject_api/__init__.py line 48 at r1 (raw file):

MediaId = NewType('MediaId', str)

DetectionComponentType = NewType('DetectionComponentType', str)

Please provide examples for the DetectionComponentType in a comment above this line.


subject/examples/PythonSubjectComponent/subject_component/__init__.py line 48 at r1 (raw file):

                entity = get_single_track_entity(track_id)
                entities.append(entity)
                relationships.append(get_relationship(entity, detection_job.media_id))

Please update this to create a relationship between this entity and the one before it. Having a relationship with two entities is more realistic.

Copy link
Copy Markdown
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brosenberg42)

Copy link
Copy Markdown
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 9 files reviewed, 1 unresolved discussion (waiting on @jrobble)


subject/api/mpf_subject_api/__init__.py line 48 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Please provide examples for the DetectionComponentType in a comment above this line.

Done.


subject/examples/PythonSubjectComponent/subject_component/__init__.py line 48 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Please update this to create a relationship between this entity and the one before it. Having a relationship with two entities is more realistic.

That will result in every entity being transitively related to every other. It will also allow entities that are never in the same media to have a relationship. Half the code will exist just to address those issues. That means the component developer needs to figure out which half of the code to ignore. I think having a simple example is better than a realistic example. If a realistic example is required, then the or-tools component is a better example.

Copy link
Copy Markdown
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brosenberg42)


subject/examples/PythonSubjectComponent/subject_component/__init__.py line 48 at r4 (raw file):

                entity = get_single_track_entity(track_id)
                entities.append(entity)
                relationships.append(get_relationship(detection_job.media_id, entity))

As discussed, please relate each entity back to one other entity. They can all be related to the same one, or hand-picked after the loop. Only important thing is that a relationship has two entities.

Copy link
Copy Markdown
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 8 of 12 files reviewed, 1 unresolved discussion (waiting on @brosenberg42 and @jrobble)


subject/examples/PythonSubjectComponent/subject_component/__init__.py line 48 at r4 (raw file):

Previously, jrobble (Jeff Robble) wrote…

As discussed, please relate each entity back to one other entity. They can all be related to the same one, or hand-picked after the loop. Only important thing is that a relationship has two entities.

Done.

Copy link
Copy Markdown
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brosenberg42)


detection/api/mpf_component_api/timing.py line 53 at r5 (raw file):

class Timing:

Why is this part if the API and not the component util?

Copy link
Copy Markdown
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jrobble)


detection/api/mpf_component_api/timing.py line 53 at r5 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Why is this part if the API and not the component util?

Because it does not rely on any outside libraries. I think we should move everything in component util that does not rely on an external library to the api package. If you agree, I'll create an issue for that.

Copy link
Copy Markdown
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brosenberg42)


detection/api/mpf_component_api/timing.py line 53 at r5 (raw file):

Previously, brosenberg42 wrote…

Because it does not rely on any outside libraries. I think we should move everything in component util that does not rely on an external library to the api package. If you agree, I'll create an issue for that.

That makes sense to me. Yes, please create an issue for it.

Copy link
Copy Markdown
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jrobble)


detection/api/mpf_component_api/timing.py line 53 at r5 (raw file):

Previously, jrobble (Jeff Robble) wrote…

That makes sense to me. Yes, please create an issue for it.

This is the issue: openmpf/openmpf#1876

@brosenberg42 brosenberg42 merged commit 4d2d30a into develop Feb 10, 2025
@brosenberg42 brosenberg42 deleted the feat/subject-tracking branch February 10, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants