[TeunHuijben]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 90.78947% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.99%. Comparing base (4a4bc2e) to head (f2ca314).

Files with missing lines	Patch %	Lines
src/funtracks/import_export/geff/_import.py	92.30%	4 Missing ⚠️
src/funtracks/import_export/geff/_export.py	77.77%	2 Missing ⚠️
src/funtracks/data_model/tracks.py	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #267      +/-   ##
==========================================
- Coverage   94.09%   93.99%   -0.10%     
==========================================
  Files          60       60              
  Lines        3521     3581      +60     
==========================================
+ Hits         3313     3366      +53     
- Misses        208      215       +7     

☔ 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.

[cmalinmayor]

@TeunHuijben @AnniekStok I think this explicitly fixes the bug we had with geff axes scaling. We were storing segmentation scale in geff axes. this is incorrect according to the geff spec, since the scale should be multiplied by the stored coordinates (in our case, leading to double scaling). And then, on import, we were never reading the geff axis scale (why we didn't see double scaling), but making it impossible to import external geffs that needed the points scaled.

Here I save the segmentation scale in tracksdata graph metadata next to shape. And then, on geff import, if there is a scale, multiply the points by that (unless it is our own erroneous geffs). Thoughts? We can still decide to shift to storing points in pixel space, as done in #262, but we will need to load the old geffs regardless, so this at least straightens up the current state first.

[TeunHuijben]

Hi @cmalinmayor, good points!

My changes:

• in _build_geff_metadata, I added, next to the version also the point_units in the metadata. Previously we only checked if version was present and made assumption on that, now I specifically save thepoint_units in the metadata with the following logic:
  • any foreign geff > pixel coordinates
  • funtracks graph, but point_units absent > world coordinates
  • fun track graph, 'point_unitspresent > readpoint_units` and decide on that (works whether we merge Keep coordinates in pixel units #262 yes or no)
• This marker is written on every export, including subgroup exports that omit the FeatureDict. Those previously carried no funtracks extra, so they were indistinguishable from foreign geffs and their world-unit points would have been scaled again.
• Fixed apply_points_scale scaling in the stored dtype: for integer pixel coordinates the scale itself got truncated (0.65 → 0), zeroing every position. Now scaled in float64.
• Export deletes graph.metadata["scale"] when tracks.scale is None, so a scale from an earlier export cannot linger (split_position_attr can hand back the live graph).

My opinions:

• I am in favor of the saving the scale in the geff.extra.tracksdata metadata, next to the shape, like you implemented
• I am in favor or saving the points in pixel coordinates (Keep coordinates in pixel units #262), to stay in line with geff spec (my previous bullet point solves how we can detect how they are saved)

[cmalinmayor]

While definitely confusing, the geff spec does not prefer pixel units - a unit of "pixel" on the axes means, these points are in pixels, and any other unit means, non-pixel units. No scale, or scale of 1, on the axes means "these coordiantes are already scaled". When designing the spec, we tried to remain agnostic, which I think just made everything confusing 😅 However, technically, the additional point_units is redundant.
Also, did you mean to put the changes on the other PR or here?

[TeunHuijben]

Hi @cmalinmayor, I now realize I fully forgot to commit/push 🤦

Most of my change you already did, thanks! The leftover that I just pushed was:

• the funtracks extra is only written when include_features=True, so subgroup exports (export_to_geff(..., node_ids=...)) come out with no funtracks marker at all. On reimport they look like a foreign geff, so their world-unit points get scaled by axes.scale a second time. The commit writes the version stamp on every export (features stays gated), widens the legacy check from "features present, no version" to "funtracks extra present, no version", and adds a regression test that a subgroup export keeps the stamp and round-trips its positions unchanged.

Good point about the unit in the geff metadata, that one should be enough to know what the units are and whether we have to convert

This PR should be ready now, and we can leave the pixel/world units discussion for the other PR