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

Implement xarray <-> dataset #56

Merged
merged 14 commits into from
Mar 24, 2020
Merged

Implement xarray <-> dataset #56

merged 14 commits into from
Mar 24, 2020

Conversation

mpinkert
Copy link
Contributor

These changes address issue #17.

This new convention supports bi-directional conversion between xarrays and datasets, under the assumption that an xarray is an image.

Currently, this does flip the axes order for C-style (default) indexed xarrays, as Java uses F-style indexing. This behavior conforms to the current status for regular numpy arrays.

Right now the conversion also assumes that the axes are linear, such that the axes can be defined with just an origin and a scale (aX + b).

Any non-numeric axes labels are currently lost (e.g., if you have coords of [R, G, B] they become [0, 1, 2] upon conversion)

thewtex
thewtex previously approved these changes Jan 2, 2020
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Awesome!

@thewtex thewtex requested a review from ctrueden January 2, 2020 21:49
@thewtex thewtex dismissed their stale review January 20, 2020 00:07

Request changes to dim names

Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

@mpinkert can we changed the dim names to lower case, which is more commonly used with xarray, Python? That is, X to x, Y to y, etc.?

@mpinkert
Copy link
Contributor Author

@thewtex the capitalized convention (X, Y, Z) is what ImageJ currently uses. I can fairly simply implement a check to auto-convert X <-> x for the xarray/python conventions, but I don't know if that would be confusing to have them different in ImageJ and in python.

I'll take a shot at it and then we can reassess. @ctrueden, do you have an opinion on this?

@ctrueden
Copy link
Member

@ctrueden, do you have an opinion on this?

In general, I am a fan of the "when in Rome" rule of development, as long as it does not conflict with the principle of least astonishment. We should try to match conventions in cases where it is practical. We can certainly have a set mapping from all the net.imagej.Axes constants to whatever the conventions are for xarray.

@mpinkert
Copy link
Contributor Author

I've implemented the requested change. Now (x, y, z, c, t) convert to (X, Y, Z, C, T) and vice versa.

@thewtex
Copy link
Member

thewtex commented Jan 24, 2020

I've implemented the requested change. Now (x, y, z, c, t) convert to (X, Y, Z, C, T) and vice versa.

Thanks!

Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

Looking awesome! See comments for what's left to tweak.

:param xarr: xarray that holds the units
:return: A list of ImageJ Axis with the specified origin and scale
"""
axes = ['']*len(xarr.dims)
Copy link
Member

Choose a reason for hiding this comment

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

If you're bored, consider using a linter (flake8? black? I don't care, but it would catch things like this).

This new convention supports bi-directional conversion between xarrays
and datasets, under the assumption that an xarray is an image.

Currently, this does flip the axes order for C-style (default) indexed
xarrays, as Java uses F-style indexing.  This behavior conforms to the
current status for regular numpy arrays.

Right now the conversion also assumes that the axes are linear, such
that the axes can be defined with just an origin and a scale (aX + b).

Any non-numeric axes labels are currently lost (e.g., if you have coords
of [R, G, B] they become [0, 1, 2] upon conversion)
The check fails for the case of float values of the coordinates, e.g.
0.05, 0.1, 0.15, due to inexact representation of floating point
arithmatic.  A diff on that access will yield results that are slightly
different, e.g. 0.10000002 and 0.99999998.  As such, I am removing this
check for now to reconsider alternate implementations.
The get_axes_coords function used to use a hard coded equation to
calculate coordinates on a linear scale.  The Java CalibratedAxis
already has this functionality with the calibratedValue() function,
which is general to any scale rather than specific.  This change.
The numpy convention places the channel axis at the third axis, despite
operating on the c-style slow-axis first.  Java also places channel
as the third axis for 2d RGB images, where possible, so this location
should not be flipped.

This check makes sure that 2D RGB images (e.g., [y, x, c] or [X, Y, C]
maintain c as the last channel.
This generalizes the 2D-RGB commit to nd images, wherever the channel
is the last commit.
This allows us to have arbitrary values for axis coordinates.  This is
not quite as good as having the appropriate axis type, because the
interpolation is linear, but allows us to preserve coordinates at
discrete points when going between datasets and xarrays.
This change now uses separate assert functions for a couple of the tests
so that they do not repeat lines.  This will make it easier to redo
tests in the future.
mpinkert and others added 6 commits March 24, 2020 10:26
This commit adds synchronization functions for moving between IJ1 and
IJ2 as well as attendant tests.

The tests currently require hard setting a fiji dir and setting
headless off, due to using a local version of imagej for testing. This
will be fixed in a future commit.
This refactoring allows the use of fixtures and the the --ij and
--headless flags.
The EnumeratedAxis is a new axis made for releases of imagej-common
later than March 2020.  This fix defaults to a LinearAxis if the
Enumerated Axis is not available, thus making the interfaces work on
older versions of imagej.
This will try headless testing on Travis, though it removes the osx
tests because osx does not have gui support.
@ctrueden
Copy link
Member

The Travis CI build did pass, even if GitHub still sees it as pending. So here goes with the merge! 🚀

@ctrueden ctrueden merged commit 5c64479 into imagej:master Mar 24, 2020
@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/installing-simpleelastix-in-python/71959/19

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