search

Open-Science-Tools / nd2reader

Pure Python library for reading NIS Elements ND2 images and metadata
http://www.lighthacking.nl/nd2reader
GNU General Public License v3.0
45 stars 28 forks source link

In 3.2.0, get_frame_vczyx() implementation breaks functionality #24

Closed WMAPernice closed 4 years ago

WMAPernice commented 4 years ago

Hi everyone,

First of all, thanks to @rbnvrw for all your work 👍 I ran into some issues in the 3.2.0 build and would love to be able to help.

Unfortunately, the current build of nd2reader (3.2.0) breaks functionality that was previously provided. This is due to the current implementation of reader.get_frame_vczyx() which replaces reader.get_frame_2D() by providing specific methods via _register_get_frame().

Problem (1) For e.g. this ND2-file a multi-FOV, multichannel, z-stack with shape (v=11, z=7, c=5, y=1806, x=1278) setting bundle_axes = 'zcyx' worked fine in 3.1.0. Now: 

with ND2Reader(path + '2019-8-14_24W-S3a-Sec1-A1_012.nd2') as images:
    images.bundle_axes = 'zcyx'
    images.iter_axes = 'v'

    print(f"Fields-of-view in ND2 file: {len(images)}")
    im = images[0]
    images.close()

print(f"Shape of single field-of-view: {im.shape}")

fails in base_frames._transpose() due to axes mismatch: 

C:\Anaconda3\envs\ND2tiff_dev\lib\site-packages\pims\base_frames.py in get_frame_T(**ind)
    300         transposition = [expected_axes.index(a) for a in desired_axes]
    301         def get_frame_T(**ind):
--> 302             return get_frame(**ind).transpose(transposition)
    303         return get_frame_T
    304 

ValueError: axes don't match array

Running with images.bundle_axes = 'czyx' instead avoids this error, but yields an object of incorrect shape:

with ND2Reader(path + '2019-8-14_24W-S3a-Sec1-A1_012.nd2') as images:
    images.bundle_axes = 'czyx'
    images.iter_axes = 'v'

    print(f"Fields-of-view in ND2 file: {len(images)}")
    im = images[0]
    images.close()

print(f"Shape of single field-of-view: {im.shape}")

Output:

Fields-of-view in ND2 file: 11
Shape of single field-of-view: (35, 1806, 1278)

This should be (5,7,1806,1278).

Bad fix: Manually calling np.reshape(im, (5,7,1806,1278)) corrects this and a possible fix could work by changing get_frames_vczyx(): https://github.com/rbnvrw/nd2reader/blob/b17c9d1c8996cf87bee1f935d9512a371387c640/nd2reader/reader.py#L70 to: 

res = np.squeeze(np.array(result, dtype=self._dtype))
return np.reshape(res, ([self.sizes[i] for i in self.bundle_axes]))

With images.bundle_axes = 'czyx' this returns the correct shape, with all frames in the right place. However, with images.bundle_axes = 'zcyx', np.reshape works incorrectly, since the order of frames in res will always follow the order of the loop in get_frame_vczyx(), aka v,c,z. I am not sure how write a fix without changing core loop (more about that at the end of this post).

Problem (2) When avoiding the axes mismatch error in baseframes._transpose() by either using images.bundle_axes = 'czyx' or with the "bad_fix", metadata is not being propagated to the individual images, as would be the case if in baseframes._make_get_frame() the "first option" fails (no appropriate register_get_frame method was defined for get_frame_dict) and _bundle() is called. As such, calling:

with ND2Reader(path + '2019-8-14_24W-S3a-Sec1-A1_012.nd2') as images:
    images.bundle_axes = 'czyx'
    images.iter_axes = 'v'
    im = images[0]
    images.close()
print(im.shape)
print(im.metadata)

yields:

(35, 1806, 1278)
{'axes': ['c', 'z', 'y', 'x'], 'coords': {'v': 0, 't': 0}}

When assembled through base_frames._bundle(), metadata is propagated and print(im.metadata) yields: 

{'height': 1806, 'width': 1278, 'date': datetime.datetime(2019, 8, 15, 10, 55, 24), 'fields_of_view': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10], 'frames': [0], 'z_levels': [0, 1, 2, 3, 4, 5, 6], 'total_images_per_channel': 77, 'channels': ['647', '488', '561', '405', 'dic'], 'pixel_microns': 0.108333333333333, 'num_frames': 1, 'experiment': {'description': 'W3D', 'loops': [{'start': 0, 'duration': 0, 'stimulation': False, 'sampling_interval': 0.0}]}, 'events': [{'index': 1, 'time': 3265.7298961281776, 'type': 7, 'name': 'Command Executed'}, {'index': 2, 'time': 4311.860809832811, 'type': 7, 'name': 'Command Executed'}, {'index': 3, 'time': 25922.38347223401, 'type': 7, 'name': 'Command Executed'}, {'index': 4, 'time': 26966.55641874671, 'type': 7, 'name': 'Command Executed'}, {'index': 5, 'time': 48852.96897947788, 'type': 7, 'name': 'Command Executed'}, {'index': 6, 'time': 49896.39902755618, 'type': 7, 'name': 'Command Executed'}, {'index': 7, 'time': 71575.21477815509, 'type': 7, 'name': 'Command Executed'}, {'index': 8, 'time': 72618.643543154, 'type': 7, 'name': 'Command Executed'}, {'index': 9, 'time': 95025.13088130951, 'type': 7, 'name': 'Command Executed'}, {'index': 10, 'time': 96067.59092727304, 'type': 7, 'name': 'Command Executed'}, {'index': 11, 'time': 117582.08049416542, 'type': 7, 'name': 'Command Executed'}, {'index': 12, 'time': 118625.93363505602, 'type': 7, 'name': 'Command Executed'}, {'index': 13, 'time': 140330.72805649042, 'type': 7, 'name': 'Command Executed'}, {'index': 14, 'time': 141398.76003962755, 'type': 7, 'name': 'Command Executed'}, {'index': 15, 'time': 163148.8205845952, 'type': 7, 'name': 'Command Executed'}, {'index': 16, 'time': 164190.54478898644, 'type': 7, 'name': 'Command Executed'}, {'index': 17, 'time': 187320.49074921012, 'type': 7, 'name': 'Command Executed'}, {'index': 18, 'time': 188363.45247617364, 'type': 7, 'name': 'Command Executed'}, {'index': 19, 'time': 211586.02401459217, 'type': 7, 'name': 'Command Executed'}, {'index': 20, 'time': 212629.77386826277, 'type': 7, 'name': 'Command Executed'}, {'index': 21, 'time': 235066.1590194106, 'type': 7, 'name': 'Command Executed'}, {'index': 22, 'time': 236108.4846636057, 'type': 7, 'name': 'Command Executed'}], 'axes': ['c', 'z', 'y', 'x'], 'coords': {'v': 0, 't': 0}}

Problem (3): Currently, get_frames_2D() is simply forwarding to get_frame_vczyx(), however the only time that get_frames_2D() would actually be called (as far as I can see) would be from within base_frames._bundle() or _drop(). Both functions will call get_frames_2D() iteratively (as they expect the original behavior of get_frames_2D(): they internally loop over the desired axes. Since this would now call the loop inside get_frame_vczyx(), it would iterate over all axes twice, and probably cause issues. I haven't tested this, since due to: https://github.com/rbnvrw/nd2reader/blob/b17c9d1c8996cf87bee1f935d9512a371387c640/nd2reader/reader.py#L167-L176 base_frames._drop() and base_frames._bundle() will pretty much never be called.

Finally a general comment: basically, in order to fix problems 1 & 2, get_frame_vczyx() must re-implement most (all?) functionality of base_frames._bundle(). I am probably missing something, but right now, it is not clear to me what the benefit of the new get_frame_vczyx() function and associated commits since 1d43617 really is. @rbnvrw is there some added functionality you are hoping to achieve? Again, I'd be happy to help 😄

rbnvrw commented 4 years ago

@WMAPernice thank you for your very detailed and extensive bug report!

I currently don't have the time to look into this, but will get back to you. There was an issue #25 related to this that is fixed in the master branch.

Very briefly, the rationale is to more closely follow the PIMS specification that uses the _register_get_frame function to register frame generation functions.

rbnvrw commented 4 years ago

@WMAPernice I was finally able to get around to looking at this issue.

The rationale is that before, ND2Reader did not respect the iter_axes and bundle_axes properties in the way it was supposed to do based on the specifications in FramesSequenceND from pims.

Therefore, I tried to make a new function that uses the _register_get_frame interface, but missed a few important points. I have now made the following changes:

Regarding to your problem (3), I'm not sure what _bundle() and _drop() functions you're referring to, because FramesSequenceND doesn't have any as far as I can see. Can you point me to the right file & line no. where I can find those functions? As far as I know, I'm now following the pims specs, but it could very well be that I'm missing something important here.

In any case, can you test if the latest master code works as expected for your files now? Since I only have a limited number of test files, I am very curious. Thanks again!

WMAPernice commented 4 years ago

Hey Ruben,

I am hosting a useful ND2 file here: https://nd2tiff.s3-us-west-2.amazonaws.com/2019-9-5_11-Multi-point-test-image_001.zip Please feel free to test changes using that file.

With commit be1be6a the results array in get_frame_vczyx() is initialized without y,x sizes - you remove them from bundle_axes before, because you want to be able to iterate over the list later. However, this way the results array does not respect y,x and does not return images. Please print out results.shape after this line and inspect: https://github.com/rbnvrw/nd2reader/blob/2a21784d620fecfc42fd74cb5e9ea4abef1e5f8a/nd2reader/reader.py#L89

In order to get get_frame_vczyx() to work properly, we will have to implement at least the functionality of _bundle() in pims base_frames._bundle(): https://github.com/soft-matter/pims/blob/71d5bda87ffa4163fd554a44614318d80e51e312/pims/base_frames.py#L256-L291

This is what is eventually called, if pims.base_frames._make_get_frame() does not encounter a pre-specified method as you now provide by setting methods with _register_get_frame(). As you see, _bundle() sets up a results array, defines a loop to iterate over and calls a method (originally get_frame_2D()) to collect the frames.

If you copy the code in _bundle() and populate get_frame_vczyx() with it (and adjust the loop), this yields correctly working function (happy to do create a PR for this). However, since we would really just recapitulate an already existing method, I am not sure if this is useful. One benefits of making get_frame_vczyx() work would be that we skip most of the computation carried out in pims.base_frames._make_get_frame(). Maybe, there are some opportunities to improve the speed/functionality beyond what _bundle() can provide and implement a better version in get_frame_vczyx(). I would be happy to help with that & testing, maybe on a dedicated dev branch?

While that might be worth exploring, could we - in the meantime - release one stable version that includes PR #22 but otherwise is based on 3.1.0 or commit 14f8d70 ? I would like to be able to use nd2reader in some other projects that depend on PR #22 but since the introduction of _register_get_frame() functions happened before that PR (in commit 1d43617) there is currently no stable version on either conda or pip that I can refer people to 😕

Let me know what you think! Again, thanks for all your efforts - I am so glad this library exists 👏

rbnvrw commented 4 years ago

Hi Wolfgang,

Turns out I was completely confused about what frame functions I needed to register - we only needed the original get_frame_2D after all and let pims figure out how to bundle the axes. I mistakenly thought I had to register functions for all possible axis combinations. Oh well.

Thank you very much for pointing this out to me and also for providing me with your very helpful file, that will be useful for testing in the future.

On the upside, I noticed that frame_no was not always added to the returned Frame object, so at least that should be fixed now.

If everything is working as expected again, please close this report and I'll make a new release ASAP.

WMAPernice commented 4 years ago

Hey Ruben - awesome. Everything seems to work fine again 👍 Closing.

WMAPernice commented 4 years ago

Hey Ruben,

a happy new year to you!

I wanted to follow-up on you releasing an updated version of nd2reader with the fixes we implemented to resolve issue #24. I would really like to be able to refer to a published version of nd2reader that is fully functional. As far as I can tell, there was no release that included these changes.

Thanks again for all your work in maintaining the library!

My best wishes to you,

Wolfgang

From: Ruben Verweij notifications@github.com Sent: Thursday, October 17, 2019 3:05 AM To: rbnvrw/nd2reader nd2reader@noreply.github.com Cc: Pernice, Wolfgang M. wp2181@cumc.columbia.edu; Mention mention@noreply.github.com Subject: Re: [rbnvrw/nd2reader] In 3.2.0, get_frame_vczyx() implementation breaks functionality (#24)

Hi Wolfgang,

Turns out I was completely confused about what frame functions I needed to register - we only needed the original get_frame_2D after all and let pims figure out how to bundle the axes. I mistakenly thought I had to register functions for all possible axis combinations. Oh well.

Thank you very much for pointing this out to me and also for providing me with your very helpful file, that will be useful for testing in the future.

On the upside, I noticed that frame_no was not always added to the returned Frame object, so at least that should be fixed now.

If everything is working as expected again, please close this report and I'll make a new release ASAP.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_rbnvrw_nd2reader_issues_24-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DAJMWEZ3GFY7ANYUPMGL5NKLQPAFEZA5CNFSM4I44SGT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBPBPAI-23issuecomment-2D543037313&d=DwMCaQ&c=G2MiLlal7SXE3PeSnG8W6_JBU6FcdVjSsBSbw6gcR0U&r=l9y3i-SASII9_bfH5jZllybBemn-cV2TU0sP7wMVlhM&m=fjM4GwUU6y8LgJq8Ahlkg5FiXNnGSlfbwPBHxVDiUy4&s=Cc9E380hv122WmFuLU5AF15oVymb5hrS1Y0Bxj8-VC4&e=, or unsubscribehttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AJMWEZ4WWKZG32CJD46K7OTQPAFEZANCNFSM4I44SGTQ&d=DwMCaQ&c=G2MiLlal7SXE3PeSnG8W6_JBU6FcdVjSsBSbw6gcR0U&r=l9y3i-SASII9_bfH5jZllybBemn-cV2TU0sP7wMVlhM&m=fjM4GwUU6y8LgJq8Ahlkg5FiXNnGSlfbwPBHxVDiUy4&s=TpGgS9SxfU2Vdfp9SiXJE1SsJgE_DvKh8meAJAjAaXo&e=.

rbnvrw commented 4 years ago

Hey Wolfgang,

Happy new year! Thank you for reminding me, unfortunately it slipped my mind.

I'm releasing a new version now. Thanks again for your efforts!

Best, Ruben

On 03-01-2020, Wolfgang Pernice wrote:

Hey Ruben,

a happy new year to you!

I wanted to follow-up on you releasing an updated version of nd2reader with the fixes we implemented to resolve issue #24. I would really like to be able to refer to a published version of nd2reader that is fully functional. As far as I can tell, there was no release that included these changes.

Thanks again for all your work in maintaining the library!

My best wishes to you,

Wolfgang

From: Ruben Verweij notifications@github.com Sent: Thursday, October 17, 2019 3:05 AM To: rbnvrw/nd2reader nd2reader@noreply.github.com Cc: Pernice, Wolfgang M. wp2181@cumc.columbia.edu; Mention mention@noreply.github.com Subject: Re: [rbnvrw/nd2reader] In 3.2.0, get_frame_vczyx() implementation breaks functionality (#24)

Hi Wolfgang,

Turns out I was completely confused about what frame functions I needed to register - we only needed the original get_frame_2D after all and let pims figure out how to bundle the axes. I mistakenly thought I had to register functions for all possible axis combinations. Oh well.

Thank you very much for pointing this out to me and also for providing me with your very helpful file, that will be useful for testing in the future.

On the upside, I noticed that frame_no was not always added to the returned Frame object, so at least that should be fixed now.

If everything is working as expected again, please close this report and I'll make a new release ASAP.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_rbnvrw_nd2reader_issues_24-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DAJMWEZ3GFY7ANYUPMGL5NKLQPAFEZA5CNFSM4I44SGT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBPBPAI-23issuecomment-2D543037313&d=DwMCaQ&c=G2MiLlal7SXE3PeSnG8W6_JBU6FcdVjSsBSbw6gcR0U&r=l9y3i-SASII9_bfH5jZllybBemn-cV2TU0sP7wMVlhM&m=fjM4GwUU6y8LgJq8Ahlkg5FiXNnGSlfbwPBHxVDiUy4&s=Cc9E380hv122WmFuLU5AF15oVymb5hrS1Y0Bxj8-VC4&e=, or unsubscribehttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AJMWEZ4WWKZG32CJD46K7OTQPAFEZANCNFSM4I44SGTQ&d=DwMCaQ&c=G2MiLlal7SXE3PeSnG8W6_JBU6FcdVjSsBSbw6gcR0U&r=l9y3i-SASII9_bfH5jZllybBemn-cV2TU0sP7wMVlhM&m=fjM4GwUU6y8LgJq8Ahlkg5FiXNnGSlfbwPBHxVDiUy4&s=TpGgS9SxfU2Vdfp9SiXJE1SsJgE_DvKh8meAJAjAaXo&e=.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/rbnvrw/nd2reader/issues/24#issuecomment-570637043