Repository navigation
Add CTC CLI oracle tests - #371
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
msschwartz21
left a comment
msschwartz21
left a comment
There was a problem hiding this comment.
Hi Ziwen! Thanks for opening this PR! This is something that we want to test.
I think I would like to limit running this test on only one OS/python combo just to reduce our CI run time. I think the easiest option is just to skip based on which OS/python version is running for a given test.
Do you know why it's currently failing on ubuntu?
Hi Morgan! I think it's a libc mismatch between the binary and the CI runner. I added workaround that passed on a local box.
I assume you want it to run locally regardless of the developer's machine platform and only skip in github actions? Is there an idiomatic way to do this in this repo? |
Honestly I don't mind if it only runs on CI. If someone really cares they can run it locally and manually toggle the skips according to their workstation. |
|
@msschwartz21 do you know why the "Test case coverage" job fails? |
It fails when the PR is opened from a fork. It's not relevant for this PR so don't worry about it. It's helpful when adding new metrics or track errors. |
| return counts | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module", params=CASES, ids=lambda c: c.name) |
There was a problem hiding this comment.
I didn't know you could parametrize fixtures! Very useful!
| return case, oracle, actual, gt_data.graph.number_of_nodes(), gt_data.graph.number_of_edges() | ||
|
|
||
|
|
||
| def test_oracle_scores_follow_from_error_counts(oracle_case): |
There was a problem hiding this comment.
Just confirming, this test case is verifying that the TRA and DET scores reported by the ctc binaries match the error counts that the ctc binaries also report?
There was a problem hiding this comment.
This is to test the _scores helper function, which is used in the "real" test case below instead of the printed output. It also sanity-checks the output parsing. So not really covering anything in the library, just here to demonstrate correctness.
msschwartz21
left a comment
msschwartz21
left a comment
There was a problem hiding this comment.
I opened issues for the two differences you uncovered so that we can address them in future PRs. Thank you Ziwen!
76a84a5
into
live-image-tracking-tools:main
Proposed Change
Add tests against the CTC CLI binaries (#184).
Currently xfail since the discrepancies might be reflecting different interpretations for the metrics.
Failing cases
Non-split
This seems to be an regression from 6afc40d. The operation-counting function still exists, but the metrics is computed from node-counting.
Single-daughter division semantics
CTC distinguishes links within a track from parent links between tracks. A parent link can exist with only one observed daughter based on the track ID. See this paper. This might have be an implementation tradeoff with
TrackingGraphnot requiring track ID.Types of Changes
What types of changes does your code introduce? Delete those that do not apply.
Which topics does your change affect? Delete those that do not apply.
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
In #184 (comment) the maintainers decided that this is not supposed to be part of CI. I'm opening a PR to have a concrete substrate for discussion.