-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
ome_metadata as provided by Tifffile is a string. Fixes #953 #998
base: master
Are you sure you want to change the base?
Conversation
ome_metadata provided by Tifffile is a String
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @mlt 🚀 I have two code-specific comments in this first round as well as a some conceptual questions:
- It sounds like this closes an open issue, is this PR related to one? If so, could you link it here using
closes: #<number>
? - If OME passes metadata as XML, how difficult would it be to parse the XML into a flat dict? A XML string works fine for
metadata
, but it would be a nice service (I think) to parse this further if possible. Do you happen to have a good reference to the spec?
Edit: I just saw that you mentioned the issue that it closes in the commit message. I will update your comment and link the issue so that it will close automatically. You can disregard my first conceptual question :)
metadata.update(flavor_metadata[0]) | ||
else: | ||
flavor_metadata = flavor_metadata[0] | ||
if isinstance(flavor_metadata, dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if isinstance(flavor_metadata, dict): | |
elif isinstance(flavor_metadata, dict): |
I think this should be elif here, right? because we are deciding how to unpack the metadata depending on it's type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. I slightly adjusted the existing code to unpack a potential dict from the tuple's first entry. Previously, there were two direct update calls. Now, if it is a tuple, we get the first entry and replace flavour_metadata with it, then actually do check if it is a dict before updating.
else: | ||
# ome only? | ||
metadata.update({flag: flavor_metadata}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain your logic here so I can follow along?
If the else
is only relevant for OME then I think we should switch it around and do something like:
if flag == 'ome':
... # ome-specific metadata loading
elif isinstance(flavor_metadata, tuple):
... # tuple-specific metadata loading
else:
... # normal metadata loading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had it akin to it originally. What did bother me was the magic "ome"... what if it changes to something. So I wanted to avoid any literal. I am not familiar with much other formats so I can't tell if "ome" is a "unicorn" or if there are others that would be non-tuple & non-dict.
Once again, if you truly believe "ome" is the only one and do not mind a literal, I do not insist on the way I wrote it. I just wanted it to be more generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I even considered ignoring non-dict entries. But it makes it slightly non-trivial to extract XML because file-level "description" is missing somehow, so one would have to write iio.immeta(filename, index=0)
to get it.
There are 2 ways to parse. We can rely on Tifffile and it's xml2dict. However, I ended up using https://github.com/tlambert03/ome-types without thinking twice. I think it is better and updateable approach. But... do you want to pull another dependency? Perhaps, it can be optional, i.e. parse with ome-types if installed otherwise pass it as is. Edit I'm unsure about flat [dict] part as it is heavily nested. |
I did some digging as to when For context: All flavor-specific metadata is handled by flavor-specific properties on the It is It is It is Based on the above, I think the best solution is a combination of both of our approaches:
Regarding the parsing of OME metadata from I looked into depending on OME-types but decided that this is not the right way forward. The library looks pretty solid, is permissively licensed (I did not check transitive dependencies), and doesn't have many dependencies. The problem is that it returns a data-class style result that is indexed via dot notation. This means that we would access OME metadata via something like meta = iio.immeta("ome.tiff")
desc = meta["ome"].images[0].description instead of using (nested) dict, which is the common approach across formats so far. I.e., users would expect something like
We could internally convert the result of OME-types into a I also looked into your suggestion of re-using TIFFfile's So in sum, I think the prudent thing to do is to return the metadata as |
Closes: #953
Previously, metadata handling expected either a tuple or presumed a dict, whereas Tifffile supplies ome metadata as a string (containing XML). Let's pass it along verbatim.