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

Enable dim_order kwargs for direct image conversions #238

Merged
merged 14 commits into from
Feb 6, 2023

Conversation

elevans
Copy link
Member

@elevans elevans commented Nov 22, 2022

This pull request enables dim_order kwargs for the direct image converter methods: to_dataset(), to_img(), and to_xarray(). It also adds tests for this new feature in test_image_conversion.py and some additional features. There are also a couple of bugs/enhancements I made (e.g. we now calculate the slope for linear axis using all points instead of the first two elements of the scale array).

To get a good idea on the dim_order kwarg option and how it works with the direct image converters, check out the new section I wrote in the docs (06-Working-with-Images, section 6.9). There is a nice table that outlines the image conversion behavior with or without dim_order supplied.

changes

  • Enable dim_order kwarg for direct image converions (to_dataset(), to_img(), to_xarray()).
  • Add tests for dim_order kwargs
  • Refactor image tests to use pytest's parameterize syntax.
  • Add test to check dimension coordinates are preserved between image conversions.
  • Replaced imagej.dims._get_scale() with imagej.dims._compute_slope(). (see latest comment)
  • Allow for dim_order lengths that are shorter than the length of the array. Unknown dims are assigned dim_n like xarray.
  • Add 6.9 -- information about using the direct image converters and dim_order.
  • Add singleton scale hack.

@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2022

Codecov Report

Base: 76.05% // Head: 77.40% // Increases project coverage by +1.35% 🎉

Coverage data is based on head (799113d) compared to base (bffc2d7).
Patch coverage: 92.96% of modified lines in pull request are covered.

📣 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     #238      +/-   ##
==========================================
+ Coverage   76.05%   77.40%   +1.35%     
==========================================
  Files          16       16              
  Lines        1829     1868      +39     
==========================================
+ Hits         1391     1446      +55     
+ Misses        438      422      -16     
Impacted Files Coverage Δ
src/imagej/stack.py 96.96% <ø> (ø)
tests/test_labeling.py 100.00% <ø> (ø)
src/imagej/dims.py 63.10% <64.28%> (+0.45%) ⬆️
src/imagej/__init__.py 62.09% <80.00%> (+1.69%) ⬆️
tests/test_image_conversion.py 98.83% <98.03%> (-1.17%) ⬇️
src/imagej/convert.py 88.62% <100.00%> (+6.46%) ⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@elevans elevans changed the title Add tests for dim_order image conversion parameter and additional bug fixes. Enable dim_order kwargs for direct image conversions Dec 5, 2022
@elevans elevans marked this pull request as ready for review December 15, 2022 21:19
@elevans elevans requested review from ctrueden and gselzer December 15, 2022 21:23
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.

I didn't take the time to understand to understand the dimension translations, but this looks like a great step forward @elevans!

Just had a couple minor suggestions, but I don't think anything was major.

@elevans
Copy link
Member Author

elevans commented Jan 30, 2023

Update:

I'm working on using xarray's global attribute's to store imagej/pyimagej specific metadata like the scale and CalibratedAxis type. See the axis-scale-logic branch for more details. The way things are going it seems this will be our preferred way to assign CalibratedAxis types. Thus I think we should scrap the scale work I've done here (i.e. _compute_scale() etc...).

This commit removes the pytest.fixtures decorator
around the image generator methods. Images are created
and thrown away during each test, defeating the purpose
of pytest.fixtures in this case. Removing the decorators
reduces code complexity.
Add direct conversion tests for to_dataset and
to_img that test dim_order arguments.
to_xarray was returning the TypeError instead
of rasing it.
Refactor the direct to_xarray tests to use
pytest's parameterize feature.
RAI's without an axis attribute (e.g. an ImgLib2 Img) raises
the TypeError because the RAI's fail an axis attribute
check (imagej.convert.supports_java_to_xarray()). This fix
converts the Img to a NumPy first and then converts the result
to a DataArray.
@elevans elevans force-pushed the dim-order-kwargs-tests branch from 0479a3c to 83d4a7e Compare February 1, 2023 16:30
This commit add a test to check if the coordinates
are the same when converting between Dataset and xarray.
This commit enables users to supply dim_orders that are
smaller in length than the number of dimensions the source data has
. The remaining unknown dimensions are labeled "dim_n" where "n"
increments with the number of unknown dimensions. This follows
xarray convention.
This commit adds image content checking (i.e.
assert that the data in one image is successfully
converted into the other) to the direct
image conversion tests.
Convert ImagePlus to ImgPlus before converting
to xarray.DataArray.
Add a new section (6.9) to working with images
describing how to use and what to expect from the
direct image converters and the dim_order argument.
Use op().run() instead of obtaining the CreateNamespace.
This is easier ot undrestand.
If the axis array is size 1 or smaller, apply a
scale of 1.
@elevans elevans force-pushed the dim-order-kwargs-tests branch from 83d4a7e to dccabff Compare February 1, 2023 17:21
@elevans
Copy link
Member Author

elevans commented Feb 1, 2023

Me again! I have reverted the dims.py module to a near original state. The difference from main is:

  • Added _validate_dim_order() function which allows users to pass dimension lists that are smaller than the size of the data. The unknown dimensions are then populated with dim_n where n is incrementing. This allows us to label unknown dimensions in the same way as xarray.
  • Added a hack to deal with converting numpy.ndarrays or xarray.DataArrays with a singleton dimension. If a singleton dimension is in the data we simply assign scale=1 for the axis. Without this hack you hit an index error when trying to do the scale math. See below for more details.

I have a nice metadata solution and a rework on how we assign axes for xarrays <-> datasets. That work is still in progress (almost done) on this branch/PR: #247

Singleton dimensions

The reason I originally added the work with axis scales was because I discovered a bug when creating more tests for the dim_order kwargs. When converting a numpy.ndarray or an xarray.DataArray that has a singleton dimension (i.e. one or more axes is size 1) we run into an index error when trying to perform a subtraction between the first and second elements of the axis/scale array here:

pyimagej/src/imagej/dims.py

Lines 277 to 289 in a5544fa

def _get_scale(axis):
"""
Get the scale of an axis, assuming it is linear and so the scale is simply
second - first coordinate.
:param axis: A 1D list like entry accessible with indexing, which contains the
axis coordinates
:return: The scale for this axis or None if it is a non-numeric scale.
"""
try:
return axis.values[1] - axis.values[0]
except TypeError:
return None

From what I gather this rather serious bug has been here the entire time. Here is how you can reproduce it on main:

import numpy as np
import imagej

ij = imagej.init()
nparr = np.random.rand(1, 2, 3, 4, 5)

# index error
ds = ij.py.to_dataset(nparr)

I think no one has ran into this yet (or very few people have) because when you slice an array to a singleton dimension its usually squashed. But anyone who is making an array with a singleton dimension will hit this bug when converting to a Dataset.

There were two blank lines it did not like that
my local Black didn't mind seeing. Who knows!
@elevans elevans requested a review from hinerm February 6, 2023 14:22
@hinerm hinerm merged commit 113841f into main Feb 6, 2023
@hinerm hinerm deleted the dim-order-kwargs-tests branch February 6, 2023 14:36
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.

5 participants