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

Consider adding delegate converter String => ImagePlus => Dataset #246

Closed
imagejan opened this issue Jun 23, 2020 · 15 comments · Fixed by #272
Closed

Consider adding delegate converter String => ImagePlus => Dataset #246

imagejan opened this issue Jun 23, 2020 · 15 comments · Fixed by #272
Assignees

Comments

@imagejan
Copy link
Member

See this forum topic.

Currently, we support:

  • String => ImagePlus via net.imagej.legacy.convert.StringToImagePlusConverter
  • ImagePlus => Dataset via net.imagej.legacy.convert.ImagePlusToDatasetConverter

In other components, we also support:

  • String => File via org.scijava.convert.FileListConverters$StringToFileConverter in scijava-common
  • File => Dataset via io.scif.convert.FileToDatasetConverter (in scifio)

and with SCIFIO 0.39.0 and later, also directly via delegate converter:

  • String (=> File) => Dataset via io.scif.convert.StringToDatasetConverter

The important difference between the two scenarios above is that String can be an image name referring to an already opened image (in case of String => ImagePlus), or String can be a file path of an image file to be opened (in case of String => File).

I think it would make sense to also support the conversion from image title to Dataset for already opened images, but we need to carefully consider the different use cases here.

@imagesc-bot
Copy link

This issue has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/plugin-with-two-datasets-parameters-will-always-pop-up-gui-in-macro-runs/36637/11

@NicoKiaru
Copy link
Contributor

Hi @imagejan,

and with SCIFIO 0.39.0 and later, also directly via delegate converter:

* `String (=> File) => Dataset` via `io.scif.convert.StringToDatasetConverter`

As mentioned in the thread I think this behaviour is particularly confusing for ij1 macro. Personally I never use Dataset in commands (I still use ImagePlus), so I do not know in which context they are the most used. For ilastik-fiji at least, this was not the wanted behaviour.

My 2 cents : I would be in favor of removing this delegate converter and making a direct String to Dataset converter - keeping in mind the ij1 macro use cases. I think converting from String to File can be explicitely done easily - so there's no big need for it.

@imagejan
Copy link
Member Author

@NicoKiaru wrote:

As mentioned in the thread I think this behaviour is particularly confusing for ij1 macro.

It was exactly an IJ1 Macro use case where this behavior was desired/requested by @tischi (see imagej/imagej-common#71 (comment) for example).

I don't have a strong opinion about this specific converter. I do think however that from the plugin/script implementer's view, you shouldn't have to worry about how people use your command, and just take a Dataset or ImagePlus as an input, and the behavior should differ whether you use one or the other.

It should be up to the framework to cover all use cases, and make it possible to work with images both read from disk or already opened, depending on the user needs (see the discussion on the imagej-common issue linked above).

@NicoKiaru
Copy link
Contributor

It was exactly an IJ1 Macro use case where this behavior was desired/requested by @tischi (see imagej/imagej-common#71 (comment) for example).

I didn't know about about this, that was why I was asking so thanks for linking, I'll read it

@NicoKiaru
Copy link
Contributor

It should be up to the framework to cover all use cases, and make it possible to work with images both read from disk or already opened, depending on the user needs (see the discussion on the imagej-common issue linked above).

I agree, but then I believe it should explicitly mentioned whether you want to allow this behaviour or not. The property style="file-or-image" is nice because then we understand what could happen. But should the default style be more restrictive ? I vote yes.

Otherwise how can you know what is the nature of the object you are refering to ? Is image.png a file located at C:\Fiji.app\imagej or the image opened and named image.png ? Is there a mechanism that prevent D:\MyStuff\MyImg.png from being a valid image name ? No I can rename an image with this name in ImageJ.

Sometimes people rename their image with the full path of the image - not saying it's good practice, but it happens.

I see a lot of ways this can create some confusing behaviour. Imagine you want to open two times the same image - maybe the second time because an image is already open it will not open it twice but only link the first opened image.

I believe a String - without specifying any sort of convention - is not enough to define what it really refers to. You also mentioning in imagej/imagej-common#71 :

The question is whether having such converters (from String to almost everything) would lead to issues somewhere else where such a conversion would not be desired.

I completely agree, and I'm pretty sure that a lot of problems can happen.

Maybe we need to adopt a convention, reinvent or reuse URL, like having a protocol name image::imagename or file::filepath. Or just reuse URL.

@imagesc-bot
Copy link

This issue has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/plugin-with-two-datasets-parameters-will-always-pop-up-gui-in-macro-runs/36637/20

@NicoKiaru
Copy link
Contributor

Just refreshing this thread, as it is pretty important. The main issue is:

  • There is no String to Dataset converter in FIJI (as of the 7 of July 2020 - if downloaded from the website)

This converter is needed in at least two cases:

For the first use case, I made this temp fix in ilastik4ij repo, which is a String to Dataset converter

For the second use case, @imagejan made a PR in scifio which will soon makes its way into FIJI

Right now these two fix live in separate worlds - and perform their duty. But they will soon be in the same world (FIJI updated to the new pom version + ilastik4ij update site), and then, when put together, issues will occur because the first fix assumes the String is a reference to an opened Image, while the second fix assumes the String is a Path.

Personnally I do not agree with the assumption String = Path, because this can create too much confusion in the behaviours. But my opinion is not important, the important point is that both use cases should work, and also, I believe, that the behaviours are easy to understand (thus easy to debug).

I saw this discussion on gitter regarding Location and LocationService. Could this be used to solve the ambiguity between a String refering to an open image or a file path ?

@imagejan @k-dominik @tischi @ctrueden

@imagejan
Copy link
Member Author

imagejan commented Jul 7, 2020

The use of Location and DataHandle is completely independent of this issue, in my opinion.

The point is that there are legitimate use cases for both applications: a String representing a file path (e.g. as a macro parameter), or a String representing an image title.

I think we can agree that #@ File parameters should be recorded as strings and we need a converter String=>File to handle option strings from those recordings. Building upon this, scifio/scifio#402 addresses the wish to use #@ Dataset parameters and get them filled by providing a file path string.

The general question whether for #@ Dataset parameters, the input harvester should always offer the possibility to load a file (in addition to listing open images) is covered by scijava/scijava-common#380, and I think we can discuss it separately from this issue.

To solve the (possible) conflict between a (FilePath)String => Dataset and an (OpenedImage)String => Dataset converter, we can do several things:

  • Improve the (FilePath)String => Dataset converter to only support conversions when the given file path actually exists.
  • Improve the (OpenedImage)String => Dataset converter to only support a conversion when the input string matches an opened image.
  • Give (OpenedImage)String => Dataset higher priority, so that an opened image is always preferred.

If we apply all three of these, then there will be little chance of conflict: only when a string matches both an existing file and an opened image, we would follow the highest priority (so there's no possibility to open the file in that case).

@ctrueden
Copy link
Member

ctrueden commented Jul 7, 2020

@NicoKiaru I agree that this is an important issue! And I thank you and @imagejan very much for discussing it, to help identify the best path forward. 💯 Unfortunately, I am currently still consumed by the server migration at the moment. I'll try to keep this issue on my radar and contribute my thoughts later... in the meantime, to help avoid the dire future you warn about coming to pass when we upload new components to the core update site, I filed fiji/fiji#252, as a place to track the concerns we should be dealing with when preparing the new Fiji release.

@NicoKiaru
Copy link
Contributor

NicoKiaru commented Jul 7, 2020

Ok, so if I understand correctly, here's the list of things to do - and the location where to do the modifications (guess) :

Would you agree @imagejan ?

@imagejan
Copy link
Member Author

imagejan commented Jul 7, 2020

Yes, exactly, @NicoKiaru.

StringToImagePlusConverter is already "well-behaved" in that it only supports conversions it can resolve with a non-null return value:

public boolean canConvert(final Object src, final Class<?> dest) {
return convert(src, ImagePlus.class) != null;
}

We just have to make sure the same is true when we migrate the converter code from ilastik4ij#StringToDatasetConverter.


As for the StringToDatasetConverter in SCIFIO, I opened scifio/scifio#434 to track progress and discuss possible adjustments.

@NicoKiaru NicoKiaru self-assigned this Jul 22, 2020
@imagesc-bot
Copy link

This issue has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/noise2void-for-fiji/34552/67

@imagesc-bot
Copy link

This issue has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/ilastik-error-on-ilastik4ij-1-7-3/46902/4

@maarzt
Copy link
Member

maarzt commented Mar 2, 2022

Let's fix this issue. Labkit has a SegmentImageWithLabkitPlugin that takes a Dataset as input. The plugin is supposed to be macro recordable. But the recorded macro often can't be executed due to this issue.

@imagejan
Copy link
Member Author

imagejan commented Mar 2, 2022

Thanks for notifying, @maarzt!

Actually, this issue (adding a StringToDatasetConverter) has been fixed in scifio/scifio#402 a while ago.
The issue mentioned by @NicoKiaru (the converter kicking in when it actually cannot and should not convert) was fixed in scifio/scifio#443.

But the recorded macro often can't be executed due to this issue.

What's the exact issue with the macro parameters? If I remember correctly, the image inputs are recorded with the image title, right? If so, we'd have to make sure that we convert title strings to Dataset, not only file paths to Dataset. But I think everything we need should exist already, so I'm not sure why it wouldn't work.

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 a pull request may close this issue.

5 participants