catalystneuro / nwb-conversion-tools

Create NWB files by converting and combining neural data in proprietary formats and adding essential metadata.
https://nwb-conversion-tools.readthedocs.io/en/main/
BSD 3-Clause "New" or "Revised" License
25 stars 12 forks source link

Scan image interface to use proper roiextractor #586

Closed h-mayorquin closed 2 years ago

h-mayorquin commented 2 years ago

Updating the ScanImageImagingInterface extractor to use ScanImageTiffImagingExtractor from roiextractors instead of TiffImagingExtractor.

h-mayorquin commented 2 years ago

Some problem with scanimage-tiff-reader==1.4.1. There are not many releases: https://pypi.org/project/scanimage-tiff-reader/#history And the activity is stalled: https://gitlab.com/vidriotech/scanimagetiffreader-python

CodyCBakerPhD commented 2 years ago

Some problem with scanimage-tiff-reader==1.4.1. There are not many releases: https://pypi.org/project/scanimage-tiff-reader/#history And the activity is stalled: https://gitlab.com/vidriotech/scanimagetiffreader-python

The problem is actually how it was built prior to release - all you have to do is install wheel in the environment prior to iteration over the requirements in our own setup.

See this line from the original PR: https://github.com/catalystneuro/roiextractors/pull/144/files#diff-7dc87d4394e1756c519dbfd0b80d3b31377f643f0bc25d3ed807ce8a3794023dR31

h-mayorquin commented 2 years ago

Some problem with scanimage-tiff-reader==1.4.1. There are not many releases: https://pypi.org/project/scanimage-tiff-reader/#history And the activity is stalled: https://gitlab.com/vidriotech/scanimagetiffreader-python

The problem is actually how it was built prior to release - all you have to do is install wheel in the environment prior to iteration over the requirements in our own setup.

See this line from the original PR: https://github.com/catalystneuro/roiextractors/pull/144/files#diff-7dc87d4394e1756c519dbfd0b80d3b31377f643f0bc25d3ed807ce8a3794023dR31

Interesting that the environments don't have wheel installed. I somehow thought this should come by default with pip.

I am concerned about the scanimage-tiff-reader package not being maintained. For example, it only tests against against Python 3.6 which does not receive security supports anymore:

https://endoflife.date/python

h-mayorquin commented 2 years ago

I was also hoping we could use this: https://neo.readthedocs.io/en/stable/io.html#neo.io.TiffIO But it does not have memmap support either

CodyCBakerPhD commented 2 years ago

I am concerned about the scanimage-tiff-reader package not being maintained. For example, it only tests against against Python 3.6 which does not receive security supports anymore:

We can always contact and request updates - however, we are now doing their job of testing against higher python versions, so as long as they don't prohibit (via upper bounds) certain dependencies like numpy, we can ensure that everything works as expected, which it appears to. Are we getting any new or obvious deprecation warnings from this?

CodyCBakerPhD commented 2 years ago

I am concerned about the scanimage-tiff-reader package not being maintained. For example, it only tests against against Python 3.6 which does not receive security supports anymore:

Also analogous to the other tiffile library we use on ROIExtractors: https://pypi.org/project/tiffile/#history

codecov[bot] commented 2 years ago

Codecov Report

Merging #586 (b838db3) into add_scan_image_test (120d593) will increase coverage by 0.01%. The diff coverage is 93.33%.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           add_scan_image_test     #586      +/-   ##
=======================================================
+ Coverage                88.32%   88.34%   +0.01%     
=======================================================
  Files                       59       59              
  Lines                     3213     3218       +5     
=======================================================
+ Hits                      2838     2843       +5     
  Misses                     375      375              
Flag Coverage Δ
unittests 88.34% <93.33%> (+0.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...faces/ophys/scanimage/scanimageimaginginterface.py 88.37% <93.33%> (+1.52%) :arrow_up:
h-mayorquin commented 2 years ago

@CodyCBakerPhD

I am concerned about the scanimage-tiff-reader package not being maintained. For example, it only tests against against Python 3.6 which does not receive security supports anymore:

We can always contact and request updates - however, we are now doing their job of testing against higher python versions, so as long as they don't prohibit (via upper bounds) certain dependencies like numpy, we can ensure that everything works as expected, which it appears to. Are we getting any new or obvious deprecation warnings from this?

I am concerned about the scanimage-tiff-reader package not being maintained. For example, it only tests against against Python 3.6 which does not receive security supports anymore:

We can always contact and request updates - however, we are now doing their job of testing against higher python versions, so as long as they don't prohibit (via upper bounds) certain dependencies like numpy, we can ensure that everything works as expected, which it appears to. Are we getting any new or obvious deprecation warnings from this?

I don't think that we are doing their job. We are testing that the library can read a specific file -that might as well be old- in some environments that we use ourselves.

Moreover, I am not coming at it from that angle. I am concerned about building infrastructure around something that might break or starts failing silently just because it does not keep up to date with the changing environment of Python and software as a whole. Given the ecological context of nwbct I feel that being reliable is more important than begin memory efficient (which is what we gain by using this library with their memmap capabilities).

Also analogous to the other tiffile library we use on ROIExtractors: https://pypi.org/project/tiffile/#history

Yes, the same argument applies. In this case even more strong as we don't gain anything by using this that' can be done by PIL for example.

In fact, I think this suggests a path forward. We could build a reliable TIFF reader based on a more reliable library like PIL or any other and then keep this specific one -by this or any other name- that allows us to do memmaped reading.