imagej / imagej/imagej-ops

OpListings ignore preallocated outputs

Open
#645 0 comments 2 reactions 1 assignee Claimed by @gselzer View on GitHub
bug
Dominant language
Java
Stars
94
Forks
44
PR merge metrics
No merged PRs in 30d

Description

There are issues with the way I wrote `OpListing`s. Shocker. 💥

imagej/napari-imagej#96 is a manifestation of one flaw of the current implementation: it does not retain any information about `ItemIO.BOTH` `ModuleItem`s. This is made clear by the [state](https://github.com/imagej/imagej-ops/blob/92837a806a9d1c61f76a74b7e5c1cd472bce6fb8/src/main/java/net/imagej/ops/OpListing.java#L62) held by the `OpListing`; we know the Op's inputs, and we know it's returns, but we *don't* know *what gets mutated along the way*. This limits the capabilities of the `OpListing`, and prevents us from accessing some functionality.

What I'd propose is instead to refactor the `OpListing` state to the following information:
* Op name
* Op functional type
* Parameter names
* Parameter types
* Parameter I/O type
* Parameter optionality
This will allow us to retain that information, and removes the need for a separate list of output types.

**EDIT: The above has been completed via #647**

We may then also need to improve [filtering](https://github.com/imagej/imagej-ops/blob/92837a806a9d1c61f76a74b7e5c1cd472bce6fb8/src/main/java/net/imagej/ops/search/OpSearcher.java#L107) step of `OpSearcher.search()` to allow for more corner cases. One case I had in mind might occur when two Ops share the same name, *reduced* pure inputs, and *reduced* outputs, but one is a `Computer` and the other is a `Function`. E.g. maybe for some number type `T` you have

`BinaryComputer ,T, RAI> math.add`
`BinaryFunction, T, RAI> math.add`

In that case, I'd want to see something like `math.add(img, number) -> img` for both resulting *reduced* `OpListing`s. Currently, we'd get two listings, as the current logic would suggest that the `Computer` has *no* return type, while the `Function` does, and thus the `distinct()` check in `OpSearcher.search()` would leave us with two `OpListings` :frowning:

But, we could get around this by being smarter, with the refactored state, and a better filter. All we really need to check to reduce N `OpListing`s to one is to ensure that the first 4 of those new variables are the same; some idea of "union"s could allow for discrepancies in the last two variables to reduce to one `OpListing`.

I'll try writing a fix to the logic soon...

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.