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

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

Open
hornm opened this issue Dec 18, 2013 · 9 comments
Open

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

hornm opened this issue Dec 18, 2013 · 9 comments
Milestone

Comments

@hornm
Copy link

hornm commented Dec 18, 2013

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

@ghost ghost assigned hinerm Dec 18, 2013
@hinerm
Copy link
Member

hinerm commented Dec 18, 2013

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!

  • Mark

@hornm
Copy link
Author

hornm commented Dec 19, 2013

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
Copy link
Member

hinerm commented Dec 19, 2013

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
Copy link
Member

hinerm commented Dec 19, 2013

@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
Copy link
Author

hornm commented Dec 20, 2013

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
Copy link
Member

hinerm commented Dec 20, 2013

@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.

dietzc added a commit to knime-ip/knip that referenced this issue Dec 28, 2013
updated according to: scifio/scifio-bf-compat#1
now it works with scifio 0.8.1 and all tests pass. we need to review this
code whether it solves the group files issue.
@hinerm
Copy link
Member

hinerm commented Jan 24, 2014

@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
Copy link
Member

Tackling the issues in numerical order, eh @hinerm? 😄

@hinerm hinerm modified the milestones: unscheduled, m1 Mar 23, 2015
@hinerm
Copy link
Member

hinerm commented Mar 23, 2015

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants