Skip to content
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

Fix a bug of parsing channel validity #59

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hayatoikoma
Copy link

This PR fixes the bug of parsing channel validity and also adds a support of another (probably, newer) metadata format.

The sample nd2 data downloaded from OME in the unit test was not correctly parsed, and the _get_channel_validity_list was returning [True, True, True, True, True, True, True] although the data has only 5 channels. Additionally, the metadata didn't have the unnecessary array format in the metadata, which made it falling back to the except statement much earlier than it should. This bug actually didn't allow us to read an image with more than 7 channels. I assumed that the unnecessary array format was used in older nd2 files, and I kept the original try statement.

I wasn't sure how you are assuring the order of validity is alined with sorted(metadata[six.b('sPlaneNew')].items() in _process_channels_metadata. As you have some comments for this sorting, I left as it is. Would you advise me if I need to touch here?

I didn't add a unit test for this fix because most unit tests seems to be using an artificially generated nd2 data. I can try to add something in test_parser.py if necessary.

Thanks for making this awesome library!

@@ -139,11 +139,15 @@ def _process_channels_metadata(self, metadata):

def _get_channel_validity_list(self, metadata):
try:
validity = self.image_metadata[six.b('SLxExperiment')][six.b('ppNextLevelEx')][six.b('')][0][
six.b('ppNextLevelEx')][six.b('')][0][six.b('pItemValid')]
validity = self.image_metadata[six.b('SLxExperiment')][six.b('ppNextLevelEx')][six.b('')][
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identical blocks of code found in 2 locations. Consider refactoring.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if it makes sense to refactor it. Please advise me if I need to try to remove this duplication. For readability, it may make sense to keep the current state.

# If none of the channels have been deleted, there is no validity list, so we just make one
validity = [True for _ in metadata]
try:
validity = self.image_metadata[six.b('SLxExperiment')][six.b('ppNextLevelEx')][six.b('')][0][
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identical blocks of code found in 2 locations. Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Nov 14, 2021

Code Climate has analyzed commit 02eeda2 and detected 0 issues on this pull request.

View more on Code Climate.

@hayatoikoma
Copy link
Author

@rbnvrw @ggirelli Any chance it can be merged at some point?

@ggirelli
Copy link
Contributor

Hej Hayato, thanks for the PR. I'm only a contributor here and I am thus unable to approve or merge PRs. @rbnvrw would be the best for that :)

@rbnvrw
Copy link
Member

rbnvrw commented Jan 15, 2025

Hi, thank you so much for the PR. This package is no longer actively developed because I'm no longer working in this field and due to various personal reasons, I can't properly maintain it any longer. If you'd like to help and become a maintainer, please reach out to me @ggirelli @hayatoikoma

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants