Add tests for correct usage of gap-closing or frame-skip edges #127
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #127 +/- ##
==========================================
+ Coverage 85.54% 89.50% +3.95%
==========================================
Files 19 19
Lines 927 886 -41
==========================================
Hits 793 793
+ Misses 134 93 -41 ☔ View full report in Codecov by Sentry. |
|
@cmalinmayor @msschwartz21 curious to get some initial feedback here on whether there's other sections/objects that we should test for compatibility with gap-closing edges, or whether there's other docs I've missed updating etc. It looks like most of our code was already implicitly handling gap closing, so this is more about enshrining it in our docs and tests. |
There was a problem hiding this comment.
This looks like a good set of initial test cases! Should each test case also be run in the opposite direction, e.g. switching gt and pred?
For the docs, I think you made the appropriate updates in what is currently written. In #116, there will be some places to add more content about the impact of frame-skip edges.
I expected we would need some changes in TrackingGraph and associated testing, but it looks like that isn't the case since everything is passing!
| G1 | ||
| 3_5 -- 3_6 -- -- -- 5_10 | ||
| 1_0 -- 1_1 -- -- -- 2_3 -- 2_4 -< | ||
| 4_5 -- 4_6 | ||
| G2 | ||
| 2_5 -- 2_6 -- -- -- 4_10 | ||
| 1_0 -- 1_1 -- 1_2 -- 1_3 -- 1_4 -< | ||
| 3_5 -- 3_6 |
There was a problem hiding this comment.
@cmalinmayor I thought we had validation in the TrackingGraph about edges only spanning one frame, but these graphs don't seem to be causing errors and I can't find this validation in the TrackingGraph. Am I misremembering or did we drop it at some point?
There was a problem hiding this comment.
I was running into the TrackingGraph validation wanting 't' for every node_id, which stemmed from my data having gaps.
There was a problem hiding this comment.
@cmalinmayor I thought we had validation in the
TrackingGraphabout edges only spanning one frame, but these graphs don't seem to be causing errors and I can't find this validation in theTrackingGraph. Am I misremembering or did we drop it at some point?
@msschwartz21 I also thought this was in the validation but I think we actually didn't add it, perhaps on accident...
There was a problem hiding this comment.
I was running into the
TrackingGraphvalidation wanting 't' for everynode_id, which stemmed from my data having gaps.
@lupinthief In my current understanding of implementing tracks with gaps, we still want all nodes to have a time associated. The gaps in time are covered by the edges that skip over frames, but still connect nodes that have times (e.g. an edge from a node in time t to a node in time t+n). Can that cover your use case as well?
|
Yes, that covers my use case, it is just that the edges were determined
from a monotonic range between 'start' and 'end', while the node attributes
were assigned from the 'detections_df' which didn't have a continuous set
of timesteps leading to empty nodes.
…On Tue, Dec 5, 2023, 19:24 Caroline Malin-Mayor ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/test_utils.py
<#127 (comment)>
:
> + G1
+ 3_5 -- 3_6 -- -- -- 5_10
+ 1_0 -- 1_1 -- -- -- 2_3 -- 2_4 -<
+ 4_5 -- 4_6
+ G2
+ 2_5 -- 2_6 -- -- -- 4_10
+ 1_0 -- 1_1 -- 1_2 -- 1_3 -- 1_4 -<
+ 3_5 -- 3_6
I was running into the TrackingGraph validation wanting 't' for every
node_id, which stemmed from my data having gaps.
@lupinthief <https://github.com/lupinthief> In my current understanding
of implementing tracks with gaps, we still want all nodes to have a time
associated. The gaps in time are covered by the edges that skip over
frames, but still connect nodes that have times (e.g. an edge from a node
in time t to a node in time t+n). Can that cover your use case as well?
—
Reply to this email directly, view it on GitHub
<#127 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4LJGE3V5JLK7AA4HPCCNTYH5YGPAVCNFSM6AAAAABAFF3LAOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONRVHE2DGOJXGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Co-authored-by: Morgan Schwartz <[email protected]>
|
@lupinthief thanks for taking a look at this and glad to hear it should work for your use case! I'm still not sure I understand how exactly your data is set up, and I'm curious to learn more.
What do |
|
Thanks for the interest. I'm loading the lineage in LBEP format, and the
CTC to graph function (I forget precise name) basically created edges
assuming a continuous np.arange between 'beginning' and 'end+1'. The
'detections_df' was missing some of those timesteps so I ended up with
exactly the situation you describe. Sorry, I'm away from my machine so
working from memory here.
…On Tue, Dec 5, 2023, 23:07 Draga Doncila Pop ***@***.***> wrote:
@lupinthief <https://github.com/lupinthief> thanks for taking a look at
this and glad to hear it should work for your use case!
I'm still not sure I understand how exactly your data is set up, and I'm
curious to learn more.
it is just that the edges were determined
from a monotonic range between 'start' and 'end', while the node attributes
were assigned from the 'detections_df' which didn't have a continuous set
of timesteps leading to empty nodes.
What do empty nodes mean here? Are they nodes that had edges connecting
them, but were for some reason not present in detections_df and therefore
had no further attributes associated, including t? If so, what causes the
discrepancy between detections_df and the nodes present in the graph?
—
Reply to this email directly, view it on GitHub
<#127 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4LJGA3QE2F2ZKQOZCRWZDYH6SKXAVCNFSM6AAAAABAFF3LAOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBRG43TCOJZGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Proposed Change
This PR adds tests to ensure gap closing edges are loaded and evaluated correctly by existing metrics and matchers, and updates language describing graphs to include definitions and discussion of such edges.
Types of Changes
What types of changes does your code introduce? Put an x in the boxes that apply.
Which topics does your change affect? Put an x in the boxes that apply.
TrackingGraph,run_metrics,cli, etc.)Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
Further Comments
Closes #115.