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 image selections #171

Closed
wants to merge 1 commit into from

Conversation

stelfrich
Copy link
Member

Previously, image selections (e.g. in a Swing UI) would show three entries in a dropdown for each image opened as a Dataset. The issue was that multiple Converters would add convertible inputs to the set of compatible inputs without checking if another converter has added a
different representation of the same data (e.g. ImageTitle, ImageDisplay, Dataset).

This commit changes the implementation in ImageTitleToImagePlusConverter to check if a Dataset is available, i.e. it prefers Datasets. Similar changes check for an available Dataset in the ImageDisplayToImagePlusConverter to prefer that.

Previously, image selections (e.g. in a Swing UI) would show three
entries in a dropdown for each image opened as a Dataset. The issue was
that multiple converters would add convertible inputs to the set of
compatible inputs without checking if another converter has added a
different representation of the same data (e.g. ImageTitle,
ImageDisplay, Dataset).

This commit changes the implementation in ImageTitleToImagePlusConverter
to check if a Dataset is available, i.e. it preferes Datasets. Similar
changes check for an available Dataset in the
ImageDisplayToImagePlusConverter to prefer that.
@stelfrich stelfrich force-pushed the fix-duplicates-in-image-selection branch from eb54a50 to 49255b6 Compare September 28, 2017 16:20
@ctrueden
Copy link
Member

@stelfrich Thank you very much for your initiative in investigating this and proposing a fix!

But oof, this is hairy stuff. I want to think a bit about whether this is really the best solution. It honestly may be—I just want to think through the implications a bit before merging it.

We could dig a bit during our weekly video chat, if you are amenable to that.

@ctrueden ctrueden self-assigned this Sep 28, 2017
@stelfrich
Copy link
Member Author

But oof, this is hairy stuff.

Trust me, I am fully aware of that. My biggest problem was that the duplicates or triplicates that are shown in the UI are actually three different objects... we could also think about a way to do the checks from this PR in the UI layer?

We could dig a bit during our weekly video chat, if you are amenable to that.

Let's do that!

@hinerm
Copy link
Member

hinerm commented Aug 19, 2024

This change is sadly insufficient.

I was unaware of this PR and a year ago made a change that is very similar but more fragile, by using String comparison.

However, let's say I have two image script parameters (using ImgPlus, while working on this issue) and two images open. If I run the script:

  1. When parsing input 1, all the converters miss until net.imagej.legacy.convert.ImagePlusToImgPlusConverter is hit and tells the ConvertService "hey we have two ImagePlus instances as options"
  2. The "first" image is arbitrarily chosen as an initial value for input 1, which triggers legacy image registration, so there is now a Dataset (and an ImageDisplay)
  3. When parsing input 2, net.imagej.convert.ImageConverters$DatasetToImgPlusConverter and net.imagej.convert.ImageConverters$ImageDisplayToImgPlusConverter now hit, because of the registered ImagePlus for input 1.
  4. The user is presented with three options: the unconverted second ImagePlus, an ImageDisplay, and a Dataset for the fconverted first input (the latter two appear identical in the list)

If the user were to select the unconverted ImagePlus as a parameter, that would result in its registration with the legacy service and future script runs would have 4 options: the repeated ImageDisplays and Datasets for both original ImagePlus instances. The ImagePlus is being hidden solely because of the aforementioned logic I added.

So the point is: we also would need this logic in DatasetToImgPlusConverter and ImageDisplayToImgPlusConverter . But that also is not sufficient.

If I change my inputs to Dataset instead of ImgPlus, I still get multiple options, this time because of
net.imagej.convert.ImageConverters$ImageDisplayToDatasetConverter and net.imagej.convert.ImageConverters$ImgToDatasetConverter.

A simple enough fix that would work now is to change all the to[Image] converters to avoid listing a particular item if we have reason to believe some other converter already is handling that item, either by:

  1. explicitly hard-coding a hierarchy (as was done in this PR - Dataset wins)
  2. taking a guess (as was done by me, excluding equivalently-named objects)

This does concern me for extensibility when creating other converters between new data types (e.g. Mat) and ImgPlus/Dataset/etc...

The reality is that these converters all need to know about each other, so maybe it would make more sense to formalize that in an extensible way. That could be disturbingly complex, though.

@hinerm
Copy link
Member

hinerm commented Sep 12, 2024

Closing in favor of fixing this upstream in scijava/scijava-common#482

@hinerm hinerm closed this Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants