AllenCellModeling / aicspylibczi

Python module utilizing libCZI for reading Zeiss CZI files.
https://allencellmodeling.github.io/aicspylibczi
GNU General Public License v3.0
36 stars 8 forks source link

Usage of libCZIrw instead of libCZI for aicsimageio #106

Closed sebi06 closed 1 year ago

sebi06 commented 2 years ago

Hi all,

I would like to get your opinion on the following topic.

From our (ZEISS) perspective it would be nice if you would consider switching from libCZI (GPL) to libCZIrw (LGPL) as the foundation for aicspylibczi, because this would allow to integrate the CZI files again directly into aicsimagio due to the more permissive license, if I understand this topic correctly.

What are you thoughts on this?

Cheers, Sebi

evamaxfield commented 2 years ago

I can only speak for myself but I just don't have the time or interest in switching over the library. That said, I would happily accept a PR to switch the library over when libczirw is released (assuming same functionality is maintained).

Long term you should know that we plan to move to a plugin model for aicsimageio. I.e. each file format / custom reader is a different install (pip install aicsimageio aicsimageio-czi) so that you or others can author your own CZI reader and not have to worry about the larger "aicsimageio library / ecosystem" that is becoming harder to maintain and license because of so many file formats.

I hope that we even get the plugin simple enough that you could build it into pylibczirw so that people could install pip install aicsimageio pylibczirw and have the same support and you would only have to maintain a single library.

sebi06 commented 2 years ago

Interesting news!

So on idea on my side is to just fork the aicspylibczi and replace the libCZI with our new libczirw and see what happens. Fram what the developers told me there should be very little changes to the reading part. Maybe we do a little internal Spike or hackathon :-)

AFAIK aicspylibczi is also using pybind11 so if the things above work, I would really like to investigate what it would take to "unify" the API from aicspylibczi and pylibczirw because to some extend they are complementary.

On a personal note, i am convinced that for us (ZEISS) it would be very beneficial to have an even more powerful python wrapper for libczirw and that it would be worth going down that route and really take the full ownership here obviously with the option to contribute. The tricky part is to justify this with a clear business case ...

toloudis commented 2 years ago

That would be amazing if you tried this replacement on a branch and it worked well. I echo Jackson's comment that we would gladly pull those changes in. It's in the best interest of this package to stay up to date. One thing to note is that we build for Windows, Linux and MacOS.

I also agree that it does make some sense for aicspylibczi and pylibczirw to come together in some way. I think it's still a big difference because of mosaic tile support? I'd love for there to be a aicsimageio Reader that just uses pylibczirw but it has to support all the access modes that aicsimageio users are used to. (e.g. smart use of dask for arrays that are too big to fit in memory)

As a prelude to the "plugin" architecture we are contemplating, just implementing a aicsimageio Reader that uses pylibczirw would be a great start.

sebi06 commented 2 years ago

I will think about it. Especially the dask topic is interesting. On a sidenote, we published also the cztile package and use it together with pylibczirw to read "only what we need". Our API is currently focused on reading any ROI from any 2D plane and we do not have the concept of a mosaic. I will discuss your input within our team.

toloudis commented 2 years ago

Personally I agree with the interface of just accessing any sub-xy region of any plane, as long as I can also ask for the metadata of the native tile sizes/boundaries, in case they are significant for the way I would read the data.

zeissmicroscopy commented 2 years ago

Hi @toloudis ,

I am not sure if I got you correctly but I think this is also what pylibCZIrw allows you to do. Of course the devil is the details and our library is still very new :-)

toloudis commented 2 years ago

Yes - whether it is the C++ or the Python, all I meant was: if a user wants to read per-tile using the old M tiles they should be able to do that.

For aicsimageio, we also just want flexibility for the dask/delayed use case to allow users to define the chunk sizes and it might make sense to use chunk sizes that match the M tile size.

sebi06 commented 2 years ago

Especially the topic with the M Index is very interesting. Basically in our team there are two opinions:

This is why we also published the cztile library.

https://pypi.org/project/cztile/

I think both approaches makes sense and this is why (one day) I would like to see both wrappers united and bases on the new libCZIrw (c++) code.

toloudis commented 1 year ago
  1. first update the C++ library being used here to the new libCZI, and make sure all tests pass etc.
  2. consider a new CZIReader implementation for aicsimageio using pylibczirw, which would not be part of this repo at all.

Task 1 should be fairly easy and do-able, to keep us on the most current code. Task 2 is much harder and the effort vs impact may leave it at a low priority for now. AICS is looking into pylibczirw for other scripting purposes like splitting multiscene czi files into single scene czi files that (hopefully) preserve metadata.

toloudis commented 1 year ago

aicspylibczi is now using ZEISS/libCZI as of version 3.1. aicspylibczi 3.1.1 has Python 3.11 support now.

I'd need a licensing expert to verify if LGPL means we can now bundle aicspylibczi with aicsimageio installs. I'll close this for now and we can add a new issue for licensing considerations.

sebi06 commented 1 year ago

Cool. I am happy to hear that you are now using the libCZI with ZSTD support etc.

I am still thinking about this pylibczirw vs. aicspylibczi topics and how to move on here.

I am open to any proposal.

toloudis commented 1 year ago

I still think the right way is to just write a aicsimageio Reader class that uses pylibczirw (call it CziReader2 for example) and then compare it with aicsimageo's CziReader..