Deprecate motile run - #447
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #447 +/- ##
==========================================
+ Coverage 88.44% 88.67% +0.23%
==========================================
Files 58 59 +1
Lines 5606 5732 +126
==========================================
+ Hits 4958 5083 +125
- Misses 648 649 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
@cmalinmayor, I think it looks good! A quick test shows that loading a geff via both paths works.
Two comments:
- In the UI, "Tracks (geff)" and "External tracks from geff" are different, but from the user's perspective, they might be hard to distinguish (how is one geff different from another geff)?
- We have to be careful with the
TracksList.tracks_loadedsignal, especially thePathpart of it. Because, depending on which import route is chosen, thePathis sometimes the.gefffile, and sometimes the root folder of a project (becausefind_geff_groupfound the geff in that folder upon loading). So listeners to the signal should be aware that they can get all kind of Paths their way.
| if isinstance(tracks, MotileRun): | ||
| directory = tracks.save(directory) | ||
| else: | ||
| write_to_geff(tracks, directory, overwrite=True) |
There was a problem hiding this comment.
Are we sure overwrite=True is good here? What if directory is a normal folder, and during loading we found a geff in there. Does this mean we only overwite the geff, or the entire directory? I think we have to be very careful, since with the new loading strategy, directory can be a lot of things (any folder containing a geff, a .zarr with a geff inside, the .geff store itself, etc.)
There was a problem hiding this comment.
Oh yeah I actually had a local unpushed commit where I made a separate dialog for the "internal" format. But I agree that this is confusing, partially because the internal GEFF save will hopefully be replaced with the persistent sql graph soon..... 🙃 I'll clean it up a bit and clarify the two paths with docstirngs and docs
…iple viewers (came up in benchmark)
|
Another change I made (that showed up in the benchmarks): The Tracks object has to actively disconnect from previous TrackViewers when displayed in a new one: "The missing disconnect was always a bug: a TracksViewer signed up for a Tracks object's signals and never signed off. Wrapping each Tracks in a fresh MotileRun hid it, because every wrapper started with no subscribers. Now that we pass the Tracks through directly, stale TracksViewers pile up on the same object and every signal updates all of them." |
|
Another point: Claude found a discrepancy in the save+load roundtrips, in that the |
|
@cmalinmayor, thanks for the changes! Should I give this a review? or are you working on it? |
I don't remember if I was done or not 😆 so go ahead and review! |
AnniekStok
left a comment
There was a problem hiding this comment.
I did a 'user' test: opening an old motile run, changing it, and saving and loading again in different formats, all worked as expected 🎉
Just reminding you that you wanted to update the docs for this as well :)
Prerequisite for splitting out napari-track-edit, which I'll do as soon as this is merged (or rather, I'll start to do and then find a bunch of other changes needed)
Design decisions: