bioio-devs / bioio

Image reading, metadata management, and image writing for Microscopy images in Python
https://bioio-devs.github.io/bioio/OVERVIEW.html
BSD 3-Clause "New" or "Revised" License
58 stars 4 forks source link

bugfix/longest-suffix-wins #55

Closed evamaxfield closed 4 months ago

evamaxfield commented 4 months ago

…ost specific plugins are matched first

Link to Relevant Issue

This pull request resolves #54

Description of Changes

Sort the list of plugins by suffix length prior to determining the plugin reader to ensure we find the most specific viable readers first

evamaxfield commented 4 months ago

Also resolves #56 by always removing leading . from suffixes but happy to reverse that and always add them instead.

evamaxfield commented 4 months ago

Updates and I think ready for review!

Here is why the sorting matters, I am not changing any of the plugin lists in any way, only sorting the order in which each key-value pair is stored in the overall cache.

Prior to this change, here was the order of the plugin cache if you simply asked for the keys. This is the order in which readers would be tested in via determine_plugin (note that we are simply looping over the plugin cache items which defaults to insert order):

Before sorting:
['tif',
 'tiff',
 'ome.tiff',
 '264',
 '265',
 '3fr',
 '3g2',
 'A64',
 'IMT',
 'MCIDAS',
 'PCX',
 'SPIDER',
 'XVTHUMB',
 'a64',
 'adp',
 'amr',
 'amv',
 'apng',
 'arw',
 'asf',
 'avc',
 'avi',
 'avs',
 'avs2',
 'bay',
 'bif',
 'bmp',
 'cdg',
 'cgi',
 'cif',
 'ct',
 'dcr',
 'dib',
 'dip',
 'dng',
 'dnxhd',
 'dv',
 'dvd',
 'erf',
 'exr',
 'fff',
 'gif',
 'icb',
 'if',
 'iiq',
 'ism',
 'jif',
 'jfif',
 'jng',
 'jp2',
 'jpg',
 'mov',
 'mp4',
 'mpo',
 'msp',
 'pdf',
 'pngppm',
 'ps',
 'zif',
 'zarr']

After this change, here is the order of the plugin cache keys:

After sorting:
['ome.tiff',
 'XVTHUMB',
 'MCIDAS',
 'SPIDER',
 'pngppm',
 'dnxhd',
 'tiff',
 'apng',
 'avs2',
 'jfif',
 'zarr',
 'tif',
 '264',
 '265',
 '3fr',
 '3g2',
 'A64',
 'IMT',
 'PCX',
 'a64',
 'adp',
 'amr',
 'amv',
 'arw',
 'asf',
 'avc',
 'avi',
 'avs',
 'bay',
 'bif',
 'bmp',
 'cdg',
 'cgi',
 'cif',
 'dcr',
 'dib',
 'dip',
 'dng',
 'dvd',
 'erf',
 'exr',
 'fff',
 'gif',
 'icb',
 'iiq',
 'ism',
 'jif',
 'jng',
 'jp2',
 'jpg',
 'mov',
 'mp4',
 'mpo',
 'msp',
 'pdf',
 'zif',
 'ct',
 'dv',
 'if',
 'ps']
BrianWhitneyAI commented 4 months ago

Is there an ome.tif plugin, dont see it on the list? I saw this error from a user BioImage does not support the image: '/Users/r.dhamrongsirivadh/Desktop/test_images/LaminB1-Variance-multiChannels-2/AICS-13_2284.ome.tif'. You may need to install an extra format dependency. See bioio README for list of some known plugins.

evamaxfield commented 4 months ago

Is there an ome.tif plugin, dont see it on the list? I saw this error from a user BioImage does not support the image: '/Users/r.dhamrongsirivadh/Desktop/test_images/LaminB1-Variance-multiChannels-2/AICS-13_2284.ome.tif'. You may need to install an extra format dependency. See bioio README for list of some known plugins.

There is an OME-TIFF plugin: https://github.com/bioio-devs/bioio-ome-tiff

That issue (if they have the ome-tiff plugin installed) is what this PR resolves. It is also ready to review @bioio-devs/trusted-developers

BrianWhitneyAI commented 4 months ago

Is there an ome.tif plugin, dont see it on the list? I saw this error from a user BioImage does not support the image: '/Users/r.dhamrongsirivadh/Desktop/test_images/LaminB1-Variance-multiChannels-2/AICS-13_2284.ome.tif'. You may need to install an extra format dependency. See bioio README for list of some known plugins.

There is an OME-TIFF plugin: https://github.com/bioio-devs/bioio-ome-tiff

That issue (if they have the ome-tiff plugin installed) is what this PR resolves. It is also ready to review @bioio-devs/trusted-developers

This is with bioio-ome-tiff installed I believe. Also which PR resolves this?

toloudis commented 4 months ago

@SeanLeRoy I kinda want to find some of our discussions about why we chose the simpler install-order over this, to make sure we are not missing some good reason for something. I almost think it would more conservative and a less dramatic change if you did this length-sorting only for when there's a "ext2.ext" situation, like keep the order the same except for making sure ome.tiff comes before tiff and ome.tif comes before tif. Are there any other extensions we know of, that this applies to?

evamaxfield commented 4 months ago

@SeanLeRoy I kinda want to find some of our discussions about why we chose the simpler install-order over this, to make sure we are not missing some good reason for something. I almost think it would more conservative and a less dramatic change if you did this length-sorting only for when there's a "ext2.ext" situation, like keep the order the same except for making sure ome.tiff comes before tiff and ome.tif comes before tif. Are there any other extensions we know of, that this applies to?

Again, to be clear. This isn't directly changing the install order. Each plugin list is still in install order, this just determines which plugin list to try first.

That is, if multiple plugins list "tif" as a supported extension, it will still try all of those plugins in their install order.

toloudis commented 4 months ago

Again, to be clear. This isn't directly changing the install order. Each plugin list is still in install order, this just determines which plugin list to try first.

That is, if multiple plugins list "tif" as a supported extension, it will still try all of those plugins in their install order.

Ohhh, I see thank you. That sounds totally fine

BrianWhitneyAI commented 4 months ago

@evamaxfield hopefully resolved all of your points in d470d7f!