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

Fix duplicate entries in input selectors #482

Merged
merged 1 commit into from
Sep 13, 2024
Merged

Conversation

hinerm
Copy link
Member

@hinerm hinerm commented Sep 6, 2024

This PR represents an alternative to this PR attempting to fix the duplicates earlier in the framework.

However, this does create a problem if the converters themselves consistently create new items. For example, this is what happens when running a trivial script multiple times:

//@ImagePlus in1
//@ImagePlus in2

println(in1)
println(in2)

image

This could be a bug in particular converters (ImageTitleToImagePlusConverter is a likely suspect) but it does raise a concern of potential behavior for other converters, too.

@hinerm
Copy link
Member Author

hinerm commented Sep 10, 2024

(ImageTitleToImagePlusConverter is a likely suspect)

OK obviously it wasn't the title converter but DatasetToImagePlusConverter which registers Datasets blindly without checking for existing mappings.

A simple fix. We also decided to just not aggressively convert items though, and instead de-duplicate on Strings

When populating potential items for an input widget, we now prefer
existing objects of a given input type to potentially convertiable
types.

Further, for convertibles we now avoid considering them if they share a
toString with any other potential input. Candidates are prioritized in
order returned by ConvertService.getCompatibleInputs.

This mitigates the potential for duplicate entries in input harvesters
when multiple input instances are convertible to the same effective
output instance.
@hinerm
Copy link
Member Author

hinerm commented Sep 10, 2024

In conjunction with these imagej-legacy changes this branch now appears to resolve the Fiji input harvester bug.

However, the String de-duplication is of course fragile, and does not address one of the goals in that inputs will change after the first run. The only way I'm aware of to satisfy the goal of consistent outputs would be to aggressively convert input candidates. That option is also working with the aforementioned imagej-legacy branch.

So we have to pick one:
a) String comparison
b) Aggressive conversion

Functionally I like b more but I am worried it will open a whole can of worms in the future as we discover more badly-behaving converters, whereas a may prove insufficient in some cases but works in the current obvious, annoying case, and could be expanded by tapping into Named, etc.

@hinerm hinerm marked this pull request as ready for review September 12, 2024 17:19
@hinerm
Copy link
Member Author

hinerm commented Sep 12, 2024

OK, decided that this is the least intrusive change (compared to converting) and we can fix the ImagePlus title elsewhere if needed

@hinerm hinerm merged commit ee1e7c1 into master Sep 13, 2024
3 checks passed
@hinerm hinerm deleted the limit-widget-inputs branch September 13, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant