Simplify linear axis assignment logic #265
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
elevans
force-pushed
the
linear-axis-logic
branch
from
145aa16
to
dd66fe0
Compare
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #265 +/- ##
==========================================
+ Coverage 76.93% 77.36% +0.42%
==========================================
Files 16 16
Lines 1869 1869
==========================================
+ Hits 1438 1446 +8
+ Misses 431 423 -8
☔ View full report in Codecov by Sentry. |
@elevans and I talked about this in person. I don't see much reason to remove EnumeratedAxis support, since it isn't hurting anything. Maybe we can just exert more pressure towards using DefaultLinearAxis, and we can use NumPy to check dimension linearity!
gselzer
force-pushed
the
linear-axis-logic
branch
from
dd66fe0
to
717229d
Compare
Mwahahahahahaha
gselzer
force-pushed
the
linear-axis-logic
branch
from
bd07bd4
to
8b281f9
Compare
The Finally block was always triggering, overwriting the EnumeratedAxis with DefaultLinearAxis.
Non-numeric scales were never handled correctly. If the coords are non-numeric, then trying to convert them to a numpy array without checking if they're numbers will fail. We now check if the coords are numeric or not and then replace them with a linear scale if they are.
hinerm
approved these changes
Jul 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I split this PR from my metadata PR (#247). This PR aims to simply how we assign calibrated linear axes to datasets. Currently we check if
net.imagej.axis.EnumeratedAxisis available and use that as our first choice. Ifnet.imagej.axis.EnumeratedAxisis not available, then we fall back tonet.imagej.axis.DefaultLinearAxis. This is fine for most things, but unfortunately this logic has some unintended consequences. @gselzer informed me that the BoneJ plugin actually checks fornet.imagej.axis.DefaultLinearAxisand is unable to use thenet.imagej.axis.EnumeratedAxis. The changes this PR brings are the following:net.imagej.axis.EnumeratedAxisfrom the axes assignment logic. While I would like to keep the option to use both, I don't realistically see how a user would indicate they wantEnumeratedAxisor not. Also if they actually care about that, then it seems likely that they may have enough knowledge to change theCalibratedAxistype. Are there instances where ImageJ2 will produce anet.imagej.Datasetwithnet.imagej.axis.EnumeratedAxis? If not, then I think that is another point for dropping this behavior.mainalready but I realize it has been passed around from one stale PR to another. Here it is! This change resolves a bug where getting the scale of a singleton dimension fails. This is because we calculate the slope from the dimensions coordinate array. The first two values are used for this math, which of course fails for singleton dimensions because their coordinate array is size1. Thus all singleton dimensions should have their scale set to1.update 1
Thanks @gselzer for adding in a good way to have both
EnumeratedAxisandDefaultLinearAxisas options based on coordinate array linearity.Things for me to do:
TODO:
update 2
I've added the following changes:
Finallyblock and slightly reworked the logic.Finallywas always triggered and replaced theEnumeratedAxiswithDefaultLinearAxis.DefaultLinearAxisEnumeratedAxisDefaultLinearAxis