Skip to content

Partial update of track IDs#158

Merged
JoOkuma merged 51 commits intoroyerlab:mainfrom
yfukai:partial_update_track_id
Sep 30, 2025
Merged

Partial update of track IDs#158
JoOkuma merged 51 commits intoroyerlab:mainfrom
yfukai:partial_update_track_id

Conversation

@yfukai
Copy link
Copy Markdown
Contributor

@yfukai yfukai commented Sep 9, 2025

This is a fresh implementation of #151. In this PR, assign_track_ids takes the node_ids argument as

rx_graph.assign_track_ids(node_ids=node_ids)

That only updates the track IDs for a subset of nodes connected to nodes specified by node_ids while maintaining the overall consistency of the ID assignment.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 9, 2025

Codecov Report

❌ Patch coverage is 83.60656% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.97%. Comparing base (8e891d2) to head (cb7fc9e).

Files with missing lines Patch % Lines
src/tracksdata/graph/_rustworkx_graph.py 81.25% 3 Missing and 3 partials ⚠️
src/tracksdata/graph/_base_graph.py 77.77% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
+ Coverage   87.78%   87.97%   +0.18%     
==========================================
  Files          50       50              
  Lines        3357     3392      +35     
  Branches      570      581      +11     
==========================================
+ Hits         2947     2984      +37     
+ Misses        248      246       -2     
  Partials      162      162              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yfukai yfukai changed the title Partial update track IDs Partial update of track IDs Sep 9, 2025
@yfukai yfukai marked this pull request as ready for review September 22, 2025 04:49
@yfukai
Copy link
Copy Markdown
Contributor Author

yfukai commented Sep 22, 2025

This is ready, I believe! @JoOkuma
This is built upon PR #162.

Copy link
Copy Markdown
Member

@JoOkuma JoOkuma left a comment

Choose a reason for hiding this comment

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

Hi @yfukai , thanks for the PR.
I really liked the part of trying to keep the same track ids.

I left some minor comments and a few questions.

assert isinstance(tracks_graph, rx.PyDiGraph)

# Check that re-assigning track IDs without reset works as expected
graph_backend.update_node_attrs(attrs={DEFAULT_ATTR_KEYS.TRACK_ID: -1})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
graph_backend.update_node_attrs(attrs={DEFAULT_ATTR_KEYS.TRACK_ID: -1})

Should we remove this line because of your comment says without reset?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment, I intended to reset the track IDs here, so that I can check in the following line starting with tracks_graph_reassign=!
Is it clearer if I split this to another test?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sounds good, no need to test.

Comment thread src/tracksdata/graph/_base_graph.py
Comment thread src/tracksdata/graph/_base_graph.py Outdated
raise NotImplementedError(f"{self.__class__.__name__} backend does not support track id assignment.")

# Internal utility to compute the exact set of nodes to assign when a subset is requested
def _compute_track_node_ids(self, seeds: list[int] | None) -> list[int]:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I really like this function, I can see some use cases besides updating track_ids
Do you think we could rename and remove the _ prefix to make it "public"?

I'm not really good at naming functions. I thought of non_branching_connected_nodes or non_branching_connected_components, but I'm open to your suggestions.

Copy link
Copy Markdown
Contributor Author

@yfukai yfukai Sep 27, 2025

Choose a reason for hiding this comment

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

Yeah, sounds great! I'm not sure which is better, but could connected_tracklet_nodes be an option, for example? I just wanted to find a shorter name. Or superset_tracklet_nodes?

Copy link
Copy Markdown
Contributor Author

@yfukai yfukai Sep 27, 2025

Choose a reason for hiding this comment

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

Or possibly tracklet_nodes or all_tracklet_nodes makes sense.

Copy link
Copy Markdown
Member

@JoOkuma JoOkuma Sep 29, 2025

Choose a reason for hiding this comment

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

@yfukai, I like tracklet_nodes the most, it's short and straight to the point, thanks for the suggestions

Comment thread src/tracksdata/graph/_rustworkx_graph.py Outdated
Comment thread src/tracksdata/graph/_rustworkx_graph.py
Comment thread src/tracksdata/graph/_test/test_graph_backends.py
yfukai and others added 3 commits September 27, 2025 08:04
Co-authored-by: Jordão Bragantini <jordao.bragantini@gmail.com>
Co-authored-by: Jordão Bragantini <jordao.bragantini@gmail.com>
Copy link
Copy Markdown
Contributor Author

@yfukai yfukai left a comment

Choose a reason for hiding this comment

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

Hi, thank you @JoOkuma for your review! I'm happy to hear you liked the implementation, which I also like 👍
I believe I addressed the issues you found, and I'm happy to have your feedback again!

assert isinstance(tracks_graph, rx.PyDiGraph)

# Check that re-assigning track IDs without reset works as expected
graph_backend.update_node_attrs(attrs={DEFAULT_ATTR_KEYS.TRACK_ID: -1})
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment, I intended to reset the track IDs here, so that I can check in the following line starting with tracks_graph_reassign=!
Is it clearer if I split this to another test?

Comment thread src/tracksdata/graph/_base_graph.py Outdated
raise NotImplementedError(f"{self.__class__.__name__} backend does not support track id assignment.")

# Internal utility to compute the exact set of nodes to assign when a subset is requested
def _compute_track_node_ids(self, seeds: list[int] | None) -> list[int]:
Copy link
Copy Markdown
Contributor Author

@yfukai yfukai Sep 27, 2025

Choose a reason for hiding this comment

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

Yeah, sounds great! I'm not sure which is better, but could connected_tracklet_nodes be an option, for example? I just wanted to find a shorter name. Or superset_tracklet_nodes?

Comment thread src/tracksdata/graph/_rustworkx_graph.py Outdated
Comment thread src/tracksdata/graph/_test/test_graph_backends.py
Comment thread src/tracksdata/graph/_rustworkx_graph.py
Comment thread src/tracksdata/graph/_test/test_graph_backends.py
Copy link
Copy Markdown
Member

@JoOkuma JoOkuma left a comment

Choose a reason for hiding this comment

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

@yfukai, LGTM, I'm approving the PR.
Feel free to merge once you rename _compute_track_node_ids function.

@yfukai
Copy link
Copy Markdown
Contributor Author

yfukai commented Sep 29, 2025

I updated the function name! I don't have the merge right but please feel free to go ahead 🙏

@JoOkuma JoOkuma merged commit af26e1c into royerlab:main Sep 30, 2025
7 checks passed
@JoOkuma
Copy link
Copy Markdown
Member

JoOkuma commented Sep 30, 2025

I forgot about that again. Thanks!

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.

3 participants