scifio / scifio-bf-compat

SCIFIO (@scifio/scifio) plug-in allowing use of Bio-Formats readers.
http://scif.io/
BSD 2-Clause "Simplified" License
3 stars 5 forks source link

setGroupFiles(...) has no effect if on wrapped BioFormats-Reader #1

Open hornm opened 10 years ago

hornm commented 10 years ago

Hi Mark, I faced a little problem when trying to enforce the a BioFormats-Reader not to group the files (e.g. CellomicsReader). The group-files property is not passed to the BioFormats-Reader and therewith will always be 'true'.

In principle before line 214 in BioFormatsFormat (reader.setId(...)) the group-property should be set (reader.setGroupFiles(..)) but, to be honest, I don't know from where to get it. Many BioFormats-Readers already need it during initialization (initFile(..)).

But as probably all BioFormats-Readers are going to be converted to scifio anyway, a fix is maybe not worthwhile ...

Best, Martin

hinerm commented 10 years ago

Hi Martin,

It will be a while before all the Bio-Formats formats are converted to SCIFIO, so it is worthwhile to me to have this working well. I think it will also improve the usability of SCIFIO in general, as we may need an easier way to expose these options.

Anyway, these flags are stored in the SCIFIO parser itself. Since both the Reader and Parser have some API overlap, I extracted some of these methods to a separate Groupable interface, which contains the setGroupFiles option you were seeking, for example.

I added a commit so that these Parser flags are now passed from SCIFIO to the Bio-Formats reader, which I believe addresses the immediate issue.

A limitation of this change is that it requires manual configuration of the Parser. So that's fine if you're constructing the individual components... but if you're using the InitializeService or going through an ImgOpener there's not a good way to configure the Parser.

I created a new ticket to address this issue. If you have any feedback based on how you're using the SCIFIO API right now, that would be great. :)

Thanks!

hornm commented 10 years ago

Hi Mark, thanks for the quick response! We are using it currently by getting a ReaderFilter through the SCIFIO instance and passing it to the ImgOpener (see the "getReader(String)"- and "getImg(String,int,Pair)"-method in https://github.com/knime-ip/knip/blob/master/org.knime.knip.io/src/org/knime/knip/io/ScifioImgSource.java, respectively). Maybe we should consider creating the Reader in a different way? We would be glad for any advice.

Best, Martin

hinerm commented 10 years ago

Hi Martin,

Ahh.. I have a couple of thoughts after looking at this code:

At line 318 you don't have to set source again. The initializeReader call takes care of it. When you get a Reader back from initializeReader - but I can see how that could be confusing if you're used to using the Bio-Formats readers, and need to call setId. :-/

I see that you did call setGroupFiles on the Reader - which is also confusing because Parsing has already occurred (during the initializeReader call).

I also realized there's a bug in the BioFormatsFormat because I don't think the Reader propagates settings, like setGroupFiles, to its delegate ImageReader. So even though you're setting the flag it's not being passed to the actual reader! Whoops...

So I will fix that, and then hopefully your code will work as-is... I'm not 100% sure how the ImageReader handles the groupFiles flag. If it doesn't work, then I'll look at refactoring the initializeReader method to pass parser configuration options as well - which almost certainly will solve this issue.

hinerm commented 10 years ago

@hornm - added this commit. Could you update to the latest scifio-bf-compat master and let me know if the files are still being erroneously grouped?

Thanks!

hornm commented 10 years ago

Hi Mark, unfortunately it's still not working as the group file-flag is not passed to the Parser.

A very ugly solution would be (just to give you an idea what the problem is) passing the group file-flag already in the initializeReader-method, than handing it over in the setSource-method and finally replacing line 217 by something like this:

Parser p = getFormat().createParser();
p.setGroupFiles(groupFiles);
setMetadata(p.parse(stream));

Hence, for me the following workaround works but with the disadvantage of parsing the file and initializing the according reader twice:

 ReaderFilter r =  ScifioGateway.getSCIFIO().initializer().initializeReader(imgRef, true);
            Parser p = r.getFormat().createParser();
            p.setGroupFiles(m_isGroupFiles);
            r.setMetadata(p.parse(imgRef));
hinerm commented 10 years ago

@hornm thanks for testing.. good to know you need to pass the parser options for sure (although it's too bad, I expected it).

There is a way to do it without the double initialization:

Format format = ScifioGateway.getSCIFIO().format().getFormat(imgRef, true);
ReaderFilter r = new ReaderFilter(format.createReader());
Parser p = format.createParser();
p.setGroupFiles(m_isGroupFiles);
r.setMetadata(p.parse(imgRef));

Note that you should NOT call setSource on the ReaderFilter because it will overwrite the Metadata. Right now, setSource is just a convenience method when you want the default settings.

It bothers me that you have to manually construct the ReaderFilter in this example.. so I might change that as part of scifio #46. And even for me it felt kind of odd just calling setMetadata on the reader.. so I made an issue for that too.

hinerm commented 10 years ago

@hornm sorry for taking so long on this. I'm basically inserting a complete configuration framework in SCIFIO, to allow modification of all the low level options that aren't easily exposed when using the convenience API. It's in development on this branch and I expect to get it merged next week.

It is quite breaking, unsurprisingly. So I will likely release 0.9.0, then update scifio-bf-compat to use the new configuration framework, at which point hopefully this issue will be resolved.

ctrueden commented 10 years ago

Tackling the issues in numerical order, eh @hinerm? :smile:

hinerm commented 9 years ago

@dietzc do you have anyone who would be able to test this easily? Is the setGroupFiles flag working for you via SCIFIOConfig or are you having problems with grouped files? (note that this was recently set to false by default to avoid costly disk i/os)