pyimagej icon indicating copy to clipboard operation
pyimagej copied to clipboard

xarray.DataArray `imagej` metadata and improved axis/scale logic

Open elevans opened this issue 2 years ago • 5 comments

This PR improves how we assign the imagej CalibratedAxis subclass of a given axis when converting an xarray.DataArray to a net.imagej.Dataset. Currently only DefaultLinearAxis and EnumeratedAxis are supported, with EnumeratedAxis being preferred if available. This behavior can be problematic as some projects expect to see a DefaultLinearAxis instead of an EnumeratedAxis. This PR changes this behavior, by assigning DefaultLinearAxis as the default axis unless the associated imagej metadata indicates otherwise. For more specific details check out this change log:

  • Added an imagej metadata attribute to xarray.DataArrays that are made via PyImageJ (i.e. results from ij.py.to_xarray() and ij.py.from_java()). The imagej metadata attribute is a dictionary that contains per axis information such as scale, origin and the CalibratedAxis subclass type.
  • Added matching support for the rest of the CalibratedAxis subclasses such as PolynomialAxis. Even though these are not used (as far as we know) by any plugins/projects the I thought it was best to be thorough and include all the subclasses. The refactoring I did for imagej.dims._assign_axes() made this addition easy.
  • Add a check for singleton dimensions when calculating scale for the fallback linear axis.
  • Refactor imagej.dims._assign_axes() (now uses the imagej metadata attribute if present to get axes information).
  • Removed deprecated private functions.
  • Misc code cleanup.

elevans avatar Jan 31 '23 20:01 elevans

Codecov Report

Patch coverage: 76.27% and project coverage change: -0.14 :warning:

Comparison is base (f02b272) 76.95% compared to head (615ee25) 76.82%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #247      +/-   ##
==========================================
- Coverage   76.95%   76.82%   -0.14%     
==========================================
  Files          16       18       +2     
  Lines        1870     1963      +93     
==========================================
+ Hits         1439     1508      +69     
- Misses        431      455      +24     
Impacted Files Coverage Δ
src/imagej/metadata/axis.py 61.53% <61.53%> (ø)
src/imagej/dims.py 61.34% <67.74%> (-1.77%) :arrow_down:
src/imagej/_java.py 81.60% <71.11%> (-5.91%) :arrow_down:
src/imagej/convert.py 84.97% <100.00%> (+0.14%) :arrow_up:
src/imagej/metadata/__init__.py 100.00% <100.00%> (ø)
tests/test_image_conversion.py 98.84% <100.00%> (+<0.01%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar Feb 04 '23 20:02 codecov-commenter

@elevans Is this ready for review now? Or are you still working on it?

ctrueden avatar Mar 10 '23 22:03 ctrueden

I'm still working on this actually. There are a few changes I want to add that I was planning to complete on my flight back home from Japan!

elevans avatar Mar 10 '23 22:03 elevans

I'm converting this PR back to a draft as it needs some re-working again before its ready for review again.

elevans avatar Apr 14 '23 15:04 elevans

I have unfortunately been busy putting out other fires and I haven't had a chance to really get this PR off the ground to where it should be in my opinion. But my time away has allowed me to think more about how to tackle this problem. So far the work I've done creates a fancy dictionary that gets attached to the xarray.DataArray's global attributes. This is fine and dandy but as soon as someone slices the array into a new shape the metadata is out of sync. This seems like a non-viable approach going forward for sophisticated image metadata for our users.

Having said all that I think moving forward the best approach is to subclass the xarray.DataArray after converting from Java and add our own methods to the array. For example I just committed https://github.com/imagej/pyimagej/pull/247/commits/ccee1b3dbe11167eb213a97c5932a3203ace77a8 which does the same isRGB check that net.imagej.DefaultDataset does. I originally added this to update an RGB key in the out going xarray.DataArray metadata dict. A better approach in my opinion would be to convert this logic to work on the xarray.DataArray itself as a method of our subclassed xarray.DataArray.

EDIT: After doing some reading the xarray team discourages subclassing xarray.DataArray because (1) its not supported and (2) there are places where internally the only the base xarray.DataArray object is returned, thus breaking your extended functionality. Thankfully the xarray team is great and has a register_dataarray_accessor() method that we can use to add our custom methods. See this documentation for more details.

elevans avatar May 18 '23 15:05 elevans