Closed kkoz closed 1 year ago
@sbesson pointed me at this PR as the possible cause of errors we're seeing on merge-ci, so I'm going to exclude this for now, since this is blocking other PR testing. Feel free to remove the exclude when you're ready - Cheers
Thanks @will-moore I have updated your comment and removed the --exclude
line to add it at the bottom of the PR description as I don't think Kevin has the permissions to update your comment. He should be able to update his description though and remove the --exclude
line to reinclude this PR in the nightly CI builds when needed.
I have come up with another way of doing this that might be easier from a build perspective. If we can add a public getter for the TileSizes
in the PixelsService
, we can just get the max plane sizes that way rather than pass them into the RawPixelsBean
constructor. I made a branch to illustrate this: https://github.com/ome/omero-romio/compare/master...kkoz:omero-romio:pixserv-sizes-getter
The changes to omero-server
would then look more like this: https://github.com/ome/omero-server/compare/master...kkoz:omero-server:tilesizes-from-pixserv
Let me know how you'd like to proceed.
Thanks @kkoz, no objection to the alternative implementation from my side.
From a process perspective the only possible issue is that the omero-server
change will likely require a version of omero-romio
with the backwards-compatible API addition to be validated by the CI.
I would propose to open the omero-romio
change, get it reviewed, merged and released especially this is is a straightforward getter addition. From there we can then propagate the change to the omero-server
component and evaluate the new functionality. Leaving @jburel and the OME team to comment on this approach as I will be on leave next week.
My 2c would be to move this to a setter to lower the API churn and if appropriate deprecate any items like omeroDataDir
and the related constructors that are unused. Generally :+1: for using the available resolutions to do things more efficiently. If this becomes a common pattern, we probably could expose this remotely, e.g. useReasonableSize()
:smile:
Currently, the
getHistogram
API only works for non-tiled images. If the image size exceedsmaxPlaneWidth * maxPlaneHeight
, then anApiUsageException
is thrown. With this change, histograms can be calculated for tiled images. The largest resolution level is chosen such thatsizeX <= maxPlaneWidth
andsizeY <= maxPlaneHeight
and the histogram is created based on those pixel values.Testing
OmeroWeb also does a size check when the webgateway/histogram_json endpoint is hit, so it cannot be used to test this change. I have written a small
click
script to test the endpoint directly: https://gist.github.com/kkoz/62bf672b0c69d7c4a3aa8844c48eb61c Without the change, this script should work fine for non-tiled images and then fail for larger images. With the change, the script should work identically for non-tiled images and produce accurate histograms for larger images.