Open marksgraham opened 1 year ago
Closing @marksgraham as it seems to be completed and merged.
Hi @vikashg afraid this is still pending so re-opening!
I can start working on SABlock
, integrating the private block and making refacto if needed if this is ok for you @marksgraham
Hi @vgrau98 yes that would be great! One thing to bear in mind is that ideally, a DiffusionModelUnet trained before the refactor should be able to load the same weights post-refactor. Happy to chat about this more if you want.
I can start working on
SABlock
, integrating the private block and making refacto if needed if this is ok for you @marksgraham
Hi @vgrau98 I'm getting back to this refactor now. Wondering if you managed to make any progress on the SABlock refactor?
@virginiafdez we should come back to the last two items here to see if we want to address anything next.
Hi @ericspod @virginiafdez, during refactoring MAISI with the latest components in the core we also find that some refactor here may not achieve similar performance as before. Such as we use one linear to replace three linear used in generative repo. Although the weights can be loaded correctly, but the result show much difference. https://github.com/Project-MONAI/MONAI/blob/139b62c4aa161969ad1126c4feeec88c9833d4ef/monai/networks/blocks/selfattention.py#L89
Before doing further refactor, I would suggest we may need to verify several tutorials to see whether we can achieve the similar the performance as before. What do you think?
After merging MONAI Generative into core issue some refactoring is needed to reduce repeat code, see discussion here.
Until this is done any blocks from Generative that are likely to be removed will be marked as private.
Will update the list of items needing refactoring here as I go along:
[x] Upsample/Downsample/Resblock/AttentionBlock in the autoencoderkl network
conv_only=True
option in Monai's block[x] SABlock and TransformerBlock used by DecoderOnlyTransformer
[x] Upsample/Downsample/BasicTransformerBlock/ResnetBlock/AttentionBlock in the diffusion_model_unet
monai.network.blocks
, similarly to the SelfAttention block that already exists therepost_conv
option to perform a convolution after the interpolation[x] SpadeNorm block - use get_norm_layer here and here
[x] SPADEAutoencoder - merge with the autoencoder KL as the only difference is in the decoder. might make more sense to just inherit from autoencoder KL (also will get the benefit of the new load_old_state_dict metho)
[x] Had to add some calls to
.contiguous()
in the diffusions model unet to stop issues with inferer tests pr here - need to dig deeper and find if these are really necessary, as these calls do copies and consume memory[x] ControlNet refactor suggested by @ericspod to tidy up some of the code here
[x] Neater init on the patchgan discriminator suggested by @ericspod here
[ ]
Schedulers - refactor some calculations into the base class as suggested by @KumoLiu [here](https://github.com/Project-MONAI/MONAI/pull/7332#discussion_r1437355600) these calculations aren't common to all the classes that inherit from scheduler (e.g. PNDM) so I'm not sure they should be moved to the base class[ ] Deferred to future after discussion with @ericspod Inferers - create new base class for the Generative infererers, see discussion here
[ ] Deferred to future after discussion with @ericspod Engines - refactor GANTrainer into a more general base class that can replace AdversarialTrainer here