Conversation
| # hdn | ||
| if self.algorithm == SupportedAlgorithm.HDN: | ||
| if self.loss.loss_type != SupportedLoss.HDN: | ||
| raise ValueError( | ||
| f"Algorithm {self.algorithm} only supports loss `hdn`." | ||
| ) | ||
| if self.model.multiscale_count > 1: | ||
| raise ValueError("Algorithm `hdn` does not support multiscale models.") |
There was a problem hiding this comment.
Hmm I see there should probably be separate child classes for MuSplit, DenoiSplit, but I guess this can wait and we create an issue
| input_shape: tuple[int, ...] = Field(default=(64, 64), validate_default=True) | ||
| """Shape of the input patch (Z, Y, X) or (Y, X) if the data is 2D.""" |
There was a problem hiding this comment.
changing this to tuple has some serialization issues with the current way we do model dump.
Note, it can actually be solved doing .model_dump(mode="json") which should automatically cast iterable python types to list.
There was a problem hiding this comment.
Interesting, I didn't know. And I guess when reading out the list, it gets casted into tuple without any issue?
Should we open an issue for refactoring the way we export the configuration? It would be nice to support tuples, that would also allow immutable defaults in functions signatures.
There was a problem hiding this comment.
Yeah default mode argument is "python", which will keep objects as python types, using "json" will convert python types to json serializable objects, and I guess there is overlap with yaml, model_dump API
| ], | ||
| predict_logvar: Literal[None, "pixelwise"], | ||
| analytical_kl: bool, | ||
| model_params: Optional[dict[str, Any]] = None, |
There was a problem hiding this comment.
What is the point of having this model_params argument? All the arguments above are none optional and will overwrite any arguments included in model_params.
| def _create_unet_based_algorithm( | ||
| axes: str, | ||
| algorithm: Literal["n2v", "care", "n2n"], | ||
| loss: Literal["n2v", "mae", "mse"], | ||
| algorithm: Literal["n2v", "care", "n2n", "hdn"], | ||
| loss: Literal["n2v", "mae", "mse", "hdn"], |
There was a problem hiding this comment.
Remove "hdn" as algorithm option in UNet-based algorithm creation function.
| algorithm : {"n2v", "care", "n2n", "hdn"} | ||
| Algorithm to use. | ||
| loss : {"n2v", "mae", "mse"} | ||
| loss : {"n2v", "mae", "mse", "hdn"} |
| def _create_vae_based_algorithm( | ||
| algorithm: Literal["hdn"], |
There was a problem hiding this comment.
Should this function be used for MuSplit and DenoiSplit algorithms as well?
There was a problem hiding this comment.
In the future probably! Currently there is no use for the convenience functions, since MicroSplit code has its own wrapper? But at some point we should have the equivalence here
| # algorithm | ||
| algorithm_params = _create_algorithm_configuration( | ||
| algorithm_params = _create_unet_based_algorithm( | ||
| axes=axes, | ||
| algorithm=algorithm, | ||
| loss=loss, |
There was a problem hiding this comment.
Maybe _create_supervised_config_dict can be renamed since now it seems to only relate to the UNet based configs? But MicroSplit algorithms are also supervised
There was a problem hiding this comment.
Agreed. Technically, we could consider that N2N is also a CARE approach. So we could choose _create_care_config_dict or the more obvious _create_unet_based_config_dict.
| if isinstance(model, VAEModule): | ||
| raise ValueError("Export of VAE models is not supported.") | ||
|
|
There was a problem hiding this comment.
Should this be a NotImplementedError with a note that we are planning to support it in the future?
| algorithm_config=self.cfg.algorithm_config, | ||
| ) | ||
| elif isinstance(self.cfg.algorithm_config, VAEBasedAlgorithm): | ||
| self.model = VAEModule( |
There was a problem hiding this comment.
Model should not even be instantiated.
|
|
||
| loss: LVAELossConfig | ||
|
|
||
| model: LVAEModel # TODO add validators |
There was a problem hiding this comment.
Can you open an issue and state what these validators should be? Otherwise you will have to figure it out again.
|
|
||
| HDN = "HDN" | ||
|
|
||
| HDN_DESCRIPTION = "" |
| from careamics.config.architectures import LVAEModel | ||
| from careamics.config.loss_model import LVAELossConfig | ||
|
|
||
| HDN = "HDN" |
There was a problem hiding this comment.
It's actually "Hierarchical DivNoising"
| # hdn | ||
| if self.algorithm == SupportedAlgorithm.HDN: | ||
| if self.loss.loss_type != SupportedLoss.HDN: | ||
| raise ValueError( | ||
| f"Algorithm {self.algorithm} only supports loss `hdn`." | ||
| ) | ||
| if self.model.multiscale_count > 1: | ||
| raise ValueError("Algorithm `hdn` does not support multiscale models.") |
There was a problem hiding this comment.
Yes they should be moved to child classes. Can you open an issue (maybe first a general MicroSplit issue, then this as a sub-issue)?
| input_shape: tuple[int, ...] = Field(default=(64, 64), validate_default=True) | ||
| """Shape of the input patch (Z, Y, X) or (Y, X) if the data is 2D.""" |
There was a problem hiding this comment.
Interesting, I didn't know. And I guess when reading out the list, it gets casted into tuple without any issue?
Should we open an issue for refactoring the way we export the configuration? It would be nice to support tuples, that would also allow immutable defaults in functions signatures.
There was a problem hiding this comment.
The python module should be renamed to vae_loss_model.py
| gaussian_likelihood: Optional[GaussianLikelihood], | ||
| noise_model_likelihood: Optional[NoiseModelLikelihood], | ||
| ) -> Optional[dict[str, torch.Tensor]]: | ||
| """Loss function for DenoiSplit. |
There was a problem hiding this comment.
This says "Loss function for DenoiSplit", it should be HDN.
| x, target = batch | ||
| x, *target = batch |
There was a problem hiding this comment.
Isn't MicroSplit using the VAEModule without this modification? In this case, why would we need it?
Also this feels totally out of PR-scope. I feel it does add an overhead in reading and understanding the code.
| masked = torch.zeros_like(batch) | ||
| mask = torch.zeros_like(batch, dtype=torch.uint8) | ||
|
|
||
| self.rng = ( |
There was a problem hiding this comment.
Why has this changed? It implies to me that every N2V Manipulate call will be the same, rather than just following the same steps each time (instantiating a new generator with the same seed each time, vs having a seeded generator created once at the beginning).
|
Superseded by #511, leaving open for reference until the other one is merged. |
|
@CatEek Do we still need this PR? Can we close it? |
4 participants
Description
Note
tldr: Minimum working HDN integrated into CAREamist.
Unfortunately, note that many of the modifications are artifacts of the merge with main. Also it's important to keep in mind that this is a mimimal example that is not supposed to be used as is, so I believe we can leave certain things up to future refactoring.
Background - why do we need this PR?
HDN is currently not available in CAREamics, but most of the features are present (Noise model, LVAE model).
It will use existing LVAE model code. The difference from MicroSplit is it is unsupervised(targets for loss computation are defined as input) and the loss function is different(but uses MIcroSplit loss under the hood)
Overview - what changed?
New configurations for HDN, but also modifying the LVAE code base to make it compatible.
New features or files
New features or files
Configuration:
HDNAlgorithmalgorithm configuration.hdninto all relevant configuration.How has this been tested?
Created a notebook in the examples repo to check performance on the BSD dataset without noise model
Related Issues
Breaking changes
Additional Notes and Examples
--- BMZ doesn't work, raises NotImplemented because the model outputs are incompatible
--- 3D isn't tested and would need another PR
--- HDN with noise model isn't tested
Please ensure your PR meets the following requirements: