Open gselzer opened 1 year ago
I'll note that yesterday I made a very hacky fix that seemed to make things work for my current use case in napari-imagej. It did, as far as I can remember, two things:
OpListing
s when they're the only Op of that name. i.e. if the only foo.bar
Op you have is a UnaryComputerOp<ByteType, ByteType>
, I don't think we should be reducing that Op to a UnaryComputerOp<RealType, RealType>
.
UnaryComputerOp<ByteType, ByteType>
and a UnaryComputerOp<LongType, LongType>
- those will still reduce down to a single UnaryComputerOp<RealType, RealType>
. But I don't know whether we should be reducing in that situation, and a negative answer to that question would make reduction more complicated.OpListing
map to reduce RealType
implementations to RealType
, not to Number
. I'm not sure why I decided to do this initially, but the effect is that Ops like image.invert(Img, Img, RealType, RealType)
can no longer be run from the searcher because those RealTypes
won't be matched by Number
s (at least here; we should be able to run this in SciJava Ops with simplification!)I may not be understanding the issue correctly, but based on how op reduction works—i.e., what the OpSearcher
gives back—the ModuleInfo info
that you get back when searching for that op will not be the actual TooMuchReductionOp
, but rather the aggregated/reduced op that includes it. So the fact that its parameters don't match the types of TooMuchReductionOp
is working-as-intended here.
In order for this op reduction thing to work, in general, we need to lean on the ConvertService
layer to take care of coercing the INPUT and BOTH types as needed. When you call the reduced op as an op, I believe that such conversions happen automatically, no? But when you use the op as a module directly like the above example, probably not. We probably shouldn't be running ops as vanilla modules, I don't think.
In order for this op reduction thing to work, in general, we need to lean on the
ConvertService
layer to take care of coercing the INPUT and BOTH types as needed.
Yup, I agree.
When you call the reduced op as an op, I believe that such conversions happen automatically, no?
I thought they did too, but that doesn't seem to be happening.
But when you use the op as a module directly like the above example, probably not. We probably shouldn't be running ops as vanilla modules, I don't think.
Keep in mind that the Module
is coming from the OpListingInfo
, so it will use ops.run
Here's a test (below) that fails. I placed it in
OpSearcherTest.java
.This test creates an Op
TooMuchReductionOp
, aUnaryComputerOp
that takes an image and aRealType
, and places its output in theRealType
.The test itself gets the
OpSearchResult
wrapping the Op, and tries to ensure that its arguments are the same type as the Op. It fails on the lineAssert.assertEquals(testRealType.getClass(), itr.next().getType());
The funny thing is that if you removed the type assertions, the test will pass, because
OpListings
are super lenient in the types they accept. But this means that GUIs like Fiji and napari-imagej will fail when trying to run Ops like this, because they are providing incorrect inputs due to the type hints.The crux of this issue is the
OpSearcher
s eagerness to reduceRealType
parameters toNumber
s. Thus Ops likeimage.invert
,imagemoments.__
, etc. are affected. The question is, what to do about it...