Skip to content

segmentation_shape metadata reformat - #260

Merged
cmalinmayor merged 15 commits into
mainfrom
fix-segmentation-shape
Aug 19, 2026
Merged

segmentation_shape metadata reformat#260
cmalinmayor merged 15 commits into
mainfrom
fix-segmentation-shape

Conversation

@TeunHuijben

@TeunHuijben TeunHuijben commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

connected to: funkelab/motile_tracker#459

Problem:

  • tracksdata saves segmentation shape in geffmetadata as .extra.tracksdata.shape
  • funtracks/motile_tracker save segmentation shape in zarr-metadata as segmentation_shape at the top level (outside the geff metadata)
  • in retrospect, the latter is not the smartest design decision, because 1) top-level non-geff metadata doesn't survive round-trips, and 2) it is inconsistent with tracksdata

Solution:
This PR removes the segmentation_shape logic, and fully uses the existing tracksdata format of saving the shape in metadata.extra.tracksdata.shape. When reading the shape from the metadata we do a double check for backwards compatibility.

For geffs that are exported from motile_tracker and then imported, it doesn't matter where we save the shape, because that is an internal discussion. But for geffs exported by other tracksdata-based tracking tools (ultrack, hoct, etc.), we are now natively compatible.

I will implement the accompanying changes in motile_tracker in a separate PR there

@TeunHuijben
TeunHuijben requested a review from cmalinmayor as a code owner July 24, 2026 23:13
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.94%. Comparing base (c30396a) to head (18c9c51).
⚠️ Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
src/funtracks/import_export/geff/_import.py 86.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #260      +/-   ##
==========================================
+ Coverage   93.92%   93.94%   +0.02%     
==========================================
  Files          59       59              
  Lines        3326     3337      +11     
==========================================
+ Hits         3124     3135      +11     
  Misses        202      202              

☔ View full report in Codecov by Harness.
📢 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.

@TeunHuijben
TeunHuijben force-pushed the fix-segmentation-shape branch from 204388f to c0af650 Compare July 27, 2026 19:37
@TeunHuijben TeunHuijben added the blocked PR blocked by something else label Jul 27, 2026
@TeunHuijben TeunHuijben removed the blocked PR blocked by something else label Jul 28, 2026
@cmalinmayor

Copy link
Copy Markdown
Contributor

Can we add an API in tracksdata for saving and loading segmentation metadata (shape and voxel size?). Then, we don't need to depend on internal tracksdata implementation details, and tracksdata can change where this info is stored at any time.

@TeunHuijben

TeunHuijben commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Hi @cmalinmayor, good point! I made the following changes:

  • tracksdata .to_geff is updated to automatically save the tracksdata metadata within the geff metadata, so we don't need to do that separately anymore
  • tracksdata shape api has been changed, so we can now just read/write graph.metadata["shape"], without needing to know how td that exactly saved in the metadata
  • a new tracksdata io helper allows reading the segmenation shape from a geff store, before loading the graph. So all of this allows us read/write/see the shape, from a surface level point of view

Some other changes along the way:

  • small change to the benchmark to run them on 1 machine, for fair comparison
  • some skips in the tests_old, because they were dependend on segmentation_shape not segmentation

)


@pytest.mark.skip(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why skip instead of delete?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had claude explain it but I still curious. i think given my backward compat concerns we should just keep them for one more release

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I made it a skip

@cmalinmayor cmalinmayor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To ensure no breaking motile_tracker:
Write both keys for one release cycle. On write, set both graph.metadata["shape"] (new,
tracksdata-aligned) and keep _update_metadata(segmentation_shape=...) (old, what
motile_tracker's solve.py/motile_run.py still write and read). This is a superset of what
the PR already does — right now it only reads the old key, it doesn't write it. If you
additionally keep writing the old key for a release or two:

  • motile_tracker's main keeps working unmodified against the new funtracks release (its
    writes still land in the old key, its reads still find the old key).
  • New tracksdata-based tools get the new key immediately.
  • You then update motile_tracker at your own pace (not tied to this PR/release), switching
    it to graph.metadata["shape"], and only after that ships do you drop the old-key write
    from funtracks.

This is the standard "dual-write, single-read-with-fallback, then cleanup" migration —
it's more code churn than a single clean cutover, but it decouples the two repos' release
trains, which is what you're actually trying to buy.

@TeunHuijben

Copy link
Copy Markdown
Collaborator Author

Hi @cmalinmayor, I agree with your solution, and keep both writes (the old and the new). This also means that all the old tests can stay intact, apart from 1 single change.

@TeunHuijben TeunHuijben added ready for final review PR ready for final approval and removed ready for initial review PR ready for first review labels Aug 10, 2026

@cmalinmayor cmalinmayor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still have some qualms with the Tracksdata API, but I'll make PRs upstream for that, and stop blocking your progress here.

EDIT: reverting my change because it was incorrect.
🍅 👍 🎉

@cmalinmayor
cmalinmayor merged commit d3664f4 into main Aug 19, 2026
27 of 28 checks passed
@cmalinmayor
cmalinmayor deleted the fix-segmentation-shape branch August 19, 2026 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for final review PR ready for final approval

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants