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 channel as second dim problem #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

acorbat
Copy link

@acorbat acorbat commented Mar 20, 2024

Following on Issue #44, I believe that the problem is that the hacky way of guessing whether Channels is the second dimension or not is summing over all the bytes of every channel, instead of looking only for the maximum. BytesInc is already accumulating the Bytes of each channel, and we should only compare the maximum with the one from the channels.

I do not have lots of images to test if this modification breaks the behaviour with other images, but it does fix the problem with my images.

@AlanWu886
Copy link

AlanWu886 commented May 13, 2024

Hi @acorbat,

Thanks for fix!
I feel your fix definitely make way more sense than mine posted in issue #44.

Do you think would it be more precise to do this?

                 if 3 in dims_dict.keys() and len(dims) > 2:
                    channels = item.findall("./Data/Image/ImageDescription/"
                                            "Channels/ChannelDescription")

                    channel_bytes_inc_list = [int(c.attrib["BytesInc"]) for c in channels]
                    channel_max = max(channel_bytes_inc_list)
                    max_index = channel_bytes_inc_list.index(channel_max)
                    channel_max += int(dims_dict[1]*dims_dict[2] * int(channels[max_index].attrib["Resolution"]) / 8)
                    
                    bytes_inc_channel = channel_max
                    cytes_inc_z = int(dims[2].attrib["BytesInc"])

                    channel_as_second_dim = bytes_inc_channel > cytes_inc_z

                else:
                    channel_as_second_dim = False

As you say the BytesInc is accumulating, but the number does not include the byte increase of that channel itself.
Thus, I feel it would be more precise to also include the byte increase of the channel with max byte increase by:
channel_max += int(dims_dict[1]*dims_dict[2] * int(channels[max_index].attrib["Resolution"]) / 8)

Please let me not your thoughts!

Sincerely yours,
Alan

@acorbat
Copy link
Author

acorbat commented May 16, 2024

Hi,

I'm wondering about the right way... I don't have an image to test this right now. I am wondering if cytes_inc_z is also the first byte. Can these be the same in any case? Are they the same when channel is NOT the second dimension? I'm sorry I can't check this right now.

On the other hand, I was wondering if you can get the step size in bytes from channel_bytes_inc_list[0] or channel_bytes_inc_list[1] as the first element might be the step size as well and you already have it.

Best,
Agustin

@AlanWu886
Copy link

Hi Agustin,

Sorry for the late response. Just want to share my thoughts regarding the this issue.

Hi,

I'm wondering about the right way... I don't have an image to test this right now. I am wondering if cytes_inc_z is also the first byte. Can these be the same in any case? Are they the same when channel is NOT the second dimension? I'm sorry I can't check this right now.

I am not quite sure what you mean here, but I really would like to learn about this more from you if possible.
In my humble opinion, I feel the author is trying to figure out what is the image stack like from code:

    def get_frame(self, z=0, t=0, c=0, m=0):
        """
        Gets the specified frame (z, t, c, m) from image.

        Args:
            z (int): z position
            t (int): time point
            c (int): channel
            m (int): mosaic image

        Returns:
            Pillow Image object
        """
        if self.display_dims != (1, 2):
            raise ValueError("Atypical imaging experiment, please use "
                             "get_plane() instead of get_frame()")

        t = int(t)
        c = int(c)
        z = int(z)
        m = int(m)
        if z >= self.nz:
            raise ValueError("Requested Z frame doesn't exist.")
        elif t >= self.nt:
            raise ValueError("Requested T frame doesn't exist.")
        elif c >= self.channels:
            raise ValueError("Requested channel doesn't exist.")
        elif m >= self.n_mosaic:
            raise ValueError("Requested mosaic image doesn't exist.")

        total_items = self.channels * self.nz * self.nt * self.n_mosaic

        t_offset = self.channels * self.nz
        t_requested = t_offset * t

        # Hack-y fix for channel as the second dim
        if self.channel_as_second_dim:
            z_requested = z
            c_offset = self.nz
            c_requested = c * c_offset
        else:
            z_offset = self.channels
            z_requested = z_offset * z
            c_requested = c

        m_offset = self.channels * self.nz * self.nt
        m_requested = m_offset * m

        item_requested = t_requested + z_requested + c_requested + m_requested
        if item_requested > total_items:
            raise ValueError("The requested item is after the end of the image")

        return self._get_item(item_requested)

Not quite sure where the evidence comes from, but It seems he is confident the stack order is either "CZTM" or "ZCTM" with t_offset = self.channels * self.nz and m_offset = self.channels * self.nz * self.nt (while "offset" here probably means the "jump of frames" in the stack), suggesting T is the third dimension and M is the last one.

On the other hand, I was wondering if you can get the step size in bytes from channel_bytes_inc_list[0] or channel_bytes_inc_list[1] as the first element might be the step size as well and you already have it.

I totally agree with your approach on this with the premise that all the channels have the same byte jump with the same size and byte depth.
Sadly, I am not an expert on microscope so I choose to be defensive here by picking the maximum then adding the byte jump of the channel with the maximum.

Best,
Alan

@acorbat
Copy link
Author

acorbat commented May 23, 2024

Hi Alan,

Regarding channel order, I understood the same thing you did. Apparently Leica used either "MTCZYX" or "MTZCYX" as dimension order (going from slowest to fastest dimension). Previously I run into this problem that C and Z dimensions were swapped so I could not read them correctly. I am not sure if dimension order is inside the metadata, but the "hack-y" solution was to look for the start byte position in the metadata and find which one is highest, we can see whether C or Z is the second dimension.

For the last point, I agree with you that the safe choice is to calculate the Byte step as you suggest. I was proposing a lazy way as it should always be the same size.

Cheers,
Agus

@acorbat
Copy link
Author

acorbat commented Jul 23, 2024

Hi Alan,

was this PR ok? Should we revisit it?

Cheers,
Agus

@AlanWu886
Copy link

Hi Agus,

It looks perfect to me! I think we just need to wait for Nick to grant the request now!

Thanks!
Alan

@keithchev
Copy link
Member

@acorbat thanks for working on this! I wanted to let you know that Nick has recently transferred ownership of this repo to Arcadia Science, since we have the bandwidth to maintain this package. We're working on going through the open PRs and issues and hope to get this PR reviewed and merged in the next week or so! Thanks again, and let me know if you have any questions.

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