search

imagej / imagej-legacy

ImageJ+ImageJ2 compatibility layer
https://imagej.net/libs/imagej-legacy
BSD 2-Clause "Simplified" License
16 stars 25 forks source link

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

Closed imagejan closed 2 years ago

imagejan commented 4 years ago

See this forum topic.

Currently, we support:

In other components, we also support:

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

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 https://github.com/imagej/imagej-common/issues/71#issuecomment-395332332 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

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 commented 4 years ago

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, https://github.com/scifio/scifio/pull/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 https://github.com/scijava/scijava-common/issues/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:

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

Yes, exactly, @NicoKiaru.

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

https://github.com/imagej/imagej-legacy/blob/5c46549e765b3148d767a8c90d15b6be03b4c154/src/main/java/net/imagej/legacy/convert/StringToImagePlusConverter.java#L71-L73

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 https://github.com/scifio/scifio/issues/434 to track progress and discuss possible adjustments.

imagesc-bot commented 4 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

Thanks for notifying, @maarzt!

Actually, this issue (adding a StringToDatasetConverter) has been fixed in https://github.com/scifio/scifio/pull/402 a while ago. The issue mentioned by @NicoKiaru (the converter kicking in when it actually cannot and should not convert) was fixed in https://github.com/scifio/scifio/pull/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.