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

Infinite pixel sizes #14

Merged
merged 1 commit into from
Jul 2, 2019
Merged

Conversation

dominikl
Copy link
Member

@dominikl dominikl commented Jun 26, 2019

Prevents that Length objects with infinite or nan value are returned from getPixelSize methods.
This can cause problems downstream (e.g in Insight).
See https://www.openmicroscopy.org/qa2/qa/feedback/27688/ or https://trello.com/c/X5egtRjc/3-iviewer-fails-to-load-images-with-pixel-size-nan-or-inf

Test: Expand the dataset newproj1/newds1 as user-3 on eel (will crash Insight without the PR).

Original stack trace:

java.lang.NumberFormatException: Infinite or NaN
	at java.math.BigDecimal.(BigDecimal.java:895)
	at java.math.BigDecimal.(BigDecimal.java:872)
	at ome.model.units.Conversion$Sym.convert(Conversion.java:271)
	at ome.model.units.Conversion$Mul.convert(Conversion.java:189)
	at omero.model.LengthI.(LengthI.java:1449)
	at omero.model.LengthI.(LengthI.java:1467)
	at omero.gateway.model.PixelsData.getPixelSizeX(PixelsData.java:261)
	at org.openmicroscopy.shoola.agents.util.EditorUtil.transformPixelsData(EditorUtil.java:717)

@jburel
Copy link
Member

jburel commented Jun 26, 2019

A similar commit should be done in the Python code. (screenshot is web)

@mtbc
Copy link
Member

mtbc commented Jun 27, 2019

Code change looks sensible.

@mtbc
Copy link
Member

mtbc commented Jun 27, 2019

Glad I got in while eel's still running. 😃

@jburel
Copy link
Member

jburel commented Jun 27, 2019

cc @will-moore

@dominikl
Copy link
Member Author

Unfortunately I have no idea where the respective code in the Python Blitzgateway is, which has to be adjusted.

@joshmoore
Copy link
Member

Merging for 5.5.3. Plan is to get integration tests green before releasing.

Note: in a future round of refactorings, I could imagine wrapping this logic into a helper method.

@joshmoore joshmoore merged commit 75bd713 into ome:master Jul 2, 2019
@chris-allan
Copy link
Member

Similar opinion on this to ome/omero-marshal#60. This applies to all floating point values across the entire model, is not something I think belongs in the gateway, and is a breaking change that should not be done in a patch release. I don't think making a single client work is justification for a fundamental API behavioural change.

The fact that the original code ignored marshaling all cases where PIXEL is the unit is already suspect.

@dominikl
Copy link
Member Author

dominikl commented Jul 4, 2019

A pixel size with unit 'pixel' doesn't really make sense.

@dominikl
Copy link
Member Author

dominikl commented Jul 4, 2019

I'll revert the PR and also remove catching the case where the pixel size unit is 'pixel'. I don't know for sure any more now, but I think there was reason for catching this (could imagine that new LengthI(value, unit) with pixel as unit isn't very happy).

@joshmoore
Copy link
Member

I added the stack trace to the description of this PR. Looking at it, I think that the conversions are disallowed but we might need to introduce Inf and NaN handling since those don't need conversion. Inf/100 is still Inf (rather than null).

@chris-allan
Copy link
Member

A pixel size with unit 'pixel' doesn't really make sense.

I agree but neither does light year or parsec either and we are not making decisions for users on the validity of using those units.

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