search

ome / omero-blitz

Gradle project containing Ice remoting code for OMERO
https://www.openmicroscopy.org/omero
GNU General Public License v2.0
0 stars 15 forks source link

model: Handle infinities in generated conversions #76

Closed joshmoore closed 4 years ago

joshmoore commented 4 years ago

If an input is an infinity, then do not perform the conversion. Throw InvalidArgumentException if nulls, PIXELS, or other unconvertible source/target pairs are passed.

see:

cc: @dominikl @jburel @mtbc

dominikl commented 4 years ago

In Insight (merge-ci build) I now get a

java.lang.NumberFormatException: Infinite or NaN
    at java.math.BigDecimal.<init>(BigDecimal.java:898)
    at java.math.BigDecimal.<init>(BigDecimal.java:875)
    at ome.model.units.Conversion$Sym.convert(Conversion.java:271)
    at ome.model.units.Conversion$Mul.convert(Conversion.java:189)
    at omero.model.LengthI.<init>(LengthI.java:1453)
    at omero.model.LengthI.<init>(LengthI.java:1471)
    at omero.gateway.model.PixelsData.getPixelSizeY(PixelsData.java:291)
    at org.openmicroscopy.shoola.agents.util.EditorUtil.transformPixelsData(EditorUtil.java:723)
    at org.openmicroscopy.shoola.agents.util.EditorUtil.formatObjectTooltip(EditorUtil.java:2531)
    at

Is that expected? But to be honest, I don't even know which gateway or blitz jar the merge-ci Insight build uses...

Btw, created an image with inf and nan pixel sizes on mergeci, Image id: 93961 (user-3)

dominikl commented 4 years ago

But the changes make sense. Might just have to handle that better in Insight. Although it's a really completely artificial case.

joshmoore commented 4 years ago

Is that expected?

from https://github.com/snoopycrimecop/omero-blitz/blob/merge_ci/src/main/java/omero/model/LengthI.java#L1453

            if (Double.isInfinite(orig)) {
                // Do nothing. Use orig
            } else {
                BigDecimal big = conversion.convert(orig); // line 1453
                converted = big.doubleValue();

If orig is a NaN in this case, then it would make sense. I'll update my PR to check for that as well.

I don't even know which gateway or blitz jar the merge-ci Insight build uses...

All the builds are chained together, so it should be testing what you expect.

dominikl commented 4 years ago

Ah of course. Sorry, yes, I created the image with both inf and nan like it was mentioned on the trello card. Don't know if that case actually needs to be handled too or can be ignored.

joshmoore commented 4 years ago

Pushed.

joshmoore commented 4 years ago

Updated with specific RuntimeException subclass (InvalidArgument) and docs.

cc: @mtbc