Skip to content

Add tests for ConvertTypes#91

Merged
gselzer merged 1 commit into
mainfrom
imagej/imagej-ops2/add-convert-type-tests
Oct 10, 2023
Merged

Add tests for ConvertTypes#91
gselzer merged 1 commit into
mainfrom
imagej/imagej-ops2/add-convert-type-tests

Conversation

@gselzer

@gselzer gselzer commented Sep 6, 2023

Copy link
Copy Markdown
Member

This PR adds in the autogenerated tests made for ImageJ Ops by @awalter17, massaged to work on ImageJ Ops2. This PR also massages/eliminates some create Ops to this end, to try and coalesce all Ops under the singular create namespace.

Points of discussion for the reviewers:

  • These tests call the Ops as functions, but the Ops are written as Computer.Arity1s (this is slick for whole-image conversion). Unfortunately, for single-type conversion, though, this is tricky to match - you can't write ops.unary("convert.float64").input(myByte).apply(), because adaptation will fail*. As a result I've added .outType calls, however I think it looks unnecessarily clunky. The only other alternative I could think of, though, would be to have both a Computer and a Function implementation for all convert Ops, which defeats the purpose of adaptation ☹️
  • Note that the first commit will fail the build because I did not solve the matching problems in the test until the second commit - we should squash the two commits before merging.

* fails because the standard Function --> Computers.Arity1 adaptors look for either a Function<ByteType, Object> or a Producer<Object>, which in the former case fails because a matched dependency makes the adaptation fail, and in the latter fails because there are multiple matches.

Closes scijava/scijava#112

@gselzer gselzer added the enhancement New feature or request label Sep 6, 2023
@gselzer gselzer requested a review from hinerm September 6, 2023 19:47
@gselzer gselzer self-assigned this Sep 6, 2023
@gselzer gselzer force-pushed the imagej/imagej-ops2/add-convert-type-tests branch 2 times, most recently from 225ec2e to 5d43a26 Compare October 10, 2023 19:51
@gselzer gselzer force-pushed the imagej/imagej-ops2/add-convert-type-tests branch from 5d43a26 to d906a39 Compare October 10, 2023 19:51
@hinerm

hinerm commented Oct 10, 2023

Copy link
Copy Markdown
Member

As a result I've added .outType calls, however I think it looks unnecessarily clunky

@gselzer agree that this is unfortunate but.. it works... it's too bad you can't just do something like:

ops.unary("convert").input(myByte).outType(Float64.class).apply()

The only other alternative I could think of, though, would be to have both a Computer and a Function implementation for all convert Ops, which defeats the purpose of adaptation

yeah let's not go down this road right now

@gselzer

gselzer commented Oct 10, 2023

Copy link
Copy Markdown
Member Author

As a result I've added .outType calls, however I think it looks unnecessarily clunky

@gselzer agree that this is unfortunate but.. it works... it's too bad you can't just do something like:

ops.unary("convert").input(myByte).outType(Float64.class).apply()

Yup, I use the Class version of outType.

The only other alternative I could think of, though, would be to have both a Computer and a Function implementation for all convert Ops, which defeats the purpose of adaptation

yeah let's not go down this road right now

Cool. Merging!

@gselzer gselzer merged commit ebc55de into main Oct 10, 2023
@hinerm hinerm deleted the imagej/imagej-ops2/add-convert-type-tests branch October 11, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

Add unit tests for ConvertTypes

2 participants