-
Notifications
You must be signed in to change notification settings - Fork 3
added cristae model and single channel transfrom #100
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
Conversation
synapse_net/inference/util.py
Outdated
| 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) |
There was a problem hiding this comment.
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.
synapse_net/inference/util.py
Outdated
| model: Optional[torch.nn.Module] = None, | ||
| verbose: bool = True, | ||
| with_channels: bool = False, | ||
| channels_to_normalize: Optional[List[int]] = [0], |
There was a problem hiding this comment.
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].
synapse_net/inference/util.py
Outdated
| # 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]) |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
synapse_net/inference/util.py
Outdated
| # 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 |
There was a problem hiding this comment.
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.
|
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