Skip to content

Conversation

@lufre1
Copy link
Contributor

@lufre1 lufre1 commented Feb 21, 2025

if with_channels:
# TODO Check that this is the correct axis.
input_volume = torch_em.transform.raw.standardize(input_volume, axis=(1, 2, 3))
input_volume = np.stack([torch_em.transform.raw.normalize(input_volume[0]), input_volume[1]], axis=0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you assume that we never want to normalize the first channel, and you assume that we always have only two channels. I don't think we should make such a general assumption.

model: Optional[torch.nn.Module] = None,
verbose: bool = True,
with_channels: bool = False,
channels_to_normalize: Optional[List[int]] = [0],
Copy link
Contributor

@constantinpape constantinpape Feb 27, 2025

Choose a reason for hiding this comment

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

Optional implies that this value can be None. What happens if it is None?
I think in this case all channels should be normalized (independently).
I would also set this as the default, and for the cristae model pass channels_to_normalize=[0].

# TODO Check that this is the correct axis.
input_volume = torch_em.transform.raw.standardize(input_volume, axis=(1, 2, 3))
for ch in channels_to_normalize:
input_volume[ch] = torch_em.transform.raw.normalize(input_volume[ch])
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave this at normalize.

Copy link
Contributor

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

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

This looks good in principle, but there is a bug, see comment.

# If we have channels then the standardization is done independently per channel.
if with_channels:
input_volume = input_volume.astype(np.float32, copy=False)
channels_to_standardize = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Now you are over-riding your input so that it's always None. If you remove this line then everything should be fine.

@constantinpape
Copy link
Contributor

Looks good and I will go ahead and merge it. (Though it doesn't yet add the model to the UI ;), but we can do this in a separate PR)

@constantinpape constantinpape merged commit 2afed55 into main Feb 28, 2025
1 check passed
@constantinpape constantinpape deleted the add-cristae-model-to-ui branch February 28, 2025 18:17
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