Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpListings ignore preallocated outputs #645

Open
gselzer opened this issue Sep 1, 2022 · 0 comments
Open

OpListings ignore preallocated outputs #645

gselzer opened this issue Sep 1, 2022 · 0 comments
Assignees
Labels
Milestone

Comments

@gselzer
Copy link
Contributor

gselzer commented Sep 1, 2022

There are issues with the way I wrote OpListings. Shocker. 💥

imagej/napari-imagej#96 is a manifestation of one flaw of the current implementation: it does not retain any information about ItemIO.BOTH ModuleItems. This is made clear by the state 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 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<ArrayImg<T> ,T, RAI<T>> math.add
BinaryFunction<CellImg<T>, T, RAI<T>> math.add

In that case, I'd want to see something like math.add(img, number) -> img for both resulting reduced OpListings. 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 😦

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 OpListings 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...

@gselzer gselzer added the bug label Sep 1, 2022
@gselzer gselzer self-assigned this Sep 1, 2022
@gselzer gselzer linked a pull request Sep 16, 2022 that will close this issue
@gselzer gselzer removed a link to a pull request Sep 16, 2022
hinerm added a commit that referenced this issue Sep 16, 2022
…d-outputs

Various OpListing Improvements.

See #645
@gselzer gselzer added this to the unscheduled milestone Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant