Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xarray.DataArray imagej metadata and improved axis/scale logic #247

Draft
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

elevans
Copy link
Member

@elevans elevans commented Jan 31, 2023

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.

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2023

Codecov Report

Patch coverage: 76.27% and project coverage change: -0.14 ⚠️

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

📣 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%) ⬇️
src/imagej/_java.py 81.60% <71.11%> (-5.91%) ⬇️
src/imagej/convert.py 84.97% <100.00%> (+0.14%) ⬆️
src/imagej/metadata/__init__.py 100.00% <100.00%> (ø)
tests/test_image_conversion.py 98.84% <100.00%> (+<0.01%) ⬆️

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

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

@elevans elevans marked this pull request as ready for review February 4, 2023 20:38
Copy link
Contributor

@gselzer gselzer left a comment

Choose a reason for hiding this comment

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

Looking pretty great! Just a couple small things I saw

src/imagej/_java.py Outdated Show resolved Hide resolved
src/imagej/convert.py Outdated Show resolved Hide resolved
src/imagej/convert.py Outdated Show resolved Hide resolved
src/imagej/dims.py Outdated Show resolved Hide resolved
src/imagej/dims.py Outdated Show resolved Hide resolved
src/imagej/dims.py Outdated Show resolved Hide resolved
@ctrueden
Copy link
Member

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

@elevans
Copy link
Member Author

elevans commented Mar 10, 2023

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 elevans marked this pull request as draft April 14, 2023 15:37
@elevans
Copy link
Member Author

elevans commented Apr 14, 2023

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

This function creates a metadata dictionary
meant to be stored in a newly created xarray.DataArray's
global attributes. The initial metadata created stores
scale type of the axis (linear, enumerated or none).
This commit changes how the linear and enumerated axes
are assigned. We now look for the "imagej" key in the
xarray's global attributes. If the key is present we look for
dim + "_axis_scale" to assign linear or enumerated axes.
Although the only calibrated axes used by nearly everyone are
DefaultLinearAxis and EnumeratedAxis, I added metadata support
for all CalibratedAxis types (e.g. PolynomialAxis etc...) just to
be thorough. This metadata will be used for matching the Calibrated
Axis type when going back to ImageJ/Java land.
This commit refactors dims._assign_axes() to use
imagej specific metadata attached to a given
xarray.DataArray. If the "imagej" attribute is present
in the xarray's global attributes then information
like scale, origin and the type of CalibratedAxis
(e.g. DefaultLinearAxis, EnumeratedAxis, etc...) can be used
to assign the correct calibrated axis per given dimension
during the net.imagej.Dataset conversion process. If the
desired calibrated axis is not available or the axis is
unknown then we fall back to a DefaultLinearAxis and attempt
to get scale/origin data from the coordinates.
The new "imagej" metadata attribute may not
always be present (depending on the origin of
the xarray). Checking for all attributes makes this
kind of hard, so lets just check for the key we put in!
If a singleton dimension is detected, assign a scale/slope
of 1.
This commit adds a new metadata submodule to handle
all image related metadata functions (e.g. creating/updating the
xarray.DataArray metadata attribute). All metadata related
functions should exist in this submodule.
Gabe suggested using a nice one-liner to get
the CalibratedAxis from the dict instead of
the if/else statement.
The axis submodule has also been streamlined to drop
the CalibratedAxis dict and instead uses a list of Strings.
This avoids Java import errors if a user imports the axis submodule
before initializing ImageJ.
The docstring wasn't clear about what kind of string to send
to and from the calibrated axis helper functions.
The axis data are stored in the scifio.metadata.image
map under the "axes" key. Converting this to a string
is unecessary.
This commit uses the metadata module to create the
metadata for both xarray and java images. Images
can now be transferred back and forth between Java
and Python without loosing the metadata.
I missed this one :(.
@elevans
Copy link
Member Author

elevans commented May 18, 2023

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 ccee1b3 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 added 17 commits May 18, 2023 11:39
This module contains the xarray accessors that
extend the xarray.DataArrays with additional methods.
This module must be imported before the accessors are found.
No other code is needed to attach these accessors.
Once the metadata has been set call the tree()
method to print a dict tree.
This preserves the metadata dict between slices.
The _update() method runs any time the MetadataAccerssor is accessed.
This allows us to update the metadata base by checking the state
of the backing xarray.DataArray. So for example if dimensions change
order or are dropped the "scifio.metadata.image" (if present) metadata
should reflect these changes where appropriate.
The axes were still in ImageJ order. They now match and check
against the dimension order of the parent xarray.DataArray.

Note that the metadata must be updated manually after creation
to update the order from Java to Python.
Use the MetadataAccessor class of the xarray to assign
the correct axes. Fallback to a DefaultLinearAxis if the metadata
is not available.
This is no longer in needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants