OpSearcher reduces OpListings too eagerly

[gselzer]

Here's a test (below) that fails. I placed it in OpSearcherTest.java.

This test creates an Op TooMuchReductionOp, a UnaryComputerOp that takes an image and a RealType, and places its output in the RealType.

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 line Assert.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 OpSearchers eagerness to reduce RealType parameters to Numbers. Thus Ops like image.invert, imagemoments.__, etc. are affected. The question is, what to do about it...

	@Plugin(type = Op.class, name = "realType.test.search")
	public static class TooMuchReductionOp extends
		AbstractUnaryComputerOp<Img<DoubleType>, DoubleType>
	{

		@Override
		public void compute(final Img<DoubleType> adsflkjsfdjlk,
			final DoubleType d)
		{
			d.set(5.0);
		}
	}

	@Test
	public void testRealTypeSearching() throws ModuleException {
		final List<SearchResult> results = searcher.search("realType.test.search", false);
		Assert.assertEquals(1, results.size());

		Img<DoubleType> testImg = ArrayImgs.doubles(10, 10);
		DoubleType testRealType = new DoubleType(10.);
		final ModuleInfo info = ((OpSearchResult) results.get(0)).info();
		Iterator<ModuleItem<?>> itr = info.inputs().iterator();
		Assert.assertEquals(testRealType.getClass(), itr.next().getType());
		Assert.assertEquals(testImg.getClass(), itr.next().getType());
		final Module module = info.createModule();
		// run the Op with unsigned bytes
		module.setInput("in", testImg);
		module.resolveInput("in");
		module.setInput("out", testRealType);
		module.resolveInput("out");

		module.run();
		Assert.assertEquals(testRealType.get(), 5.0, 1e-6);
	}

[gselzer]

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:

1. Avoids reducing OpListings 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>.
   • This doesn't fix other weird scenarios, where maybe you have a 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.
2. Refactors the default 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 Numbers (at least here; we should be able to run this in SciJava Ops with simplification!)

[ctrueden]

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.

[gselzer]

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