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

Refactor additions from Generative module #7227

Open
8 of 11 tasks
marksgraham opened this issue Nov 13, 2023 · 8 comments
Open
8 of 11 tasks

Refactor additions from Generative module #7227

marksgraham opened this issue Nov 13, 2023 · 8 comments
Assignees
Labels
enhancement New feature or request refactor Non-breaking feature enhancements

Comments

@marksgraham
Copy link
Contributor

marksgraham commented Nov 13, 2023

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:

  • Upsample/Downsample/Resblock/AttentionBlock in the autoencoderkl network

    • Upsample block can be replaced by Monai's upsample block, might need to implement conv_only=True option in Monai's block
    • Downsample block can be replaced by Monai's conv, see comment here
    • The Attention block should be able to make use of Monai's self attention block
    • I don't see a simple way to replace the Resblock, many other network's implement their own versions, but maybe we should rename it to AutoencoderKLResblock to differentiate it from other Resblocks in the codebase as done in e.g. UnerR
  • SABlock and TransformerBlock used by DecoderOnlyTransformer

    • Use the internal monai versions. could make use of the new cross-attention transformer block we plan to make as described above
  • Upsample/Downsample/BasicTransformerBlock/ResnetBlock/AttentionBlock in the diffusion_model_unet

    • The CrossAttention block could be added as a block under monai.network.blocks, similarly to the SelfAttention block that already exists there
    • The SelfAttention block can be replaced with the block we already have
    • The TransformerBlock is actually a cross-attention transformer block, we could make a new monai block for it
    • Downsample block - maybe we need to add an option to the existing monai downsample block to either conv or pool, then we could use it here
    • Upsample block - we can use monai's version if we add a post_conv option to perform a convolution after the interpolation
    • Resnet - once again I think it is OK to keep this and rename to DIffusionUnetResnetBlock
  • SpadeNorm block - use get_norm_layer here and here

  • 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)

  • 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

  • ControlNet refactor suggested by @ericspod to tidy up some of the code here

  • Neater init on the patchgan discriminator suggested by @ericspod here

  • Schedulers - refactor some calculations into the base class as suggested by @KumoLiu [here](6676 port diffusion schedulers #7332 (comment)) 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

@KumoLiu KumoLiu added enhancement New feature or request refactor Non-breaking feature enhancements labels Nov 14, 2023
@vikashg vikashg self-assigned this Dec 1, 2023
marksgraham added a commit that referenced this issue Dec 5, 2023
Partially fixes #6676 


### Description
Implements the AutoencoderKL network from MONAI Generative.

NB this network is subject to a planned refactor once the porting is
complete, [see
here](#7227).

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [x] New tests added to cover the changes.
- [x] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [x] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [x] In-line docstrings updated.
- [x] Documentation updated, tested `make html` command in the `docs/`
folder.

---------

Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: Eric Kerfoot <[email protected]>
Co-authored-by: YunLiu <[email protected]>
KumoLiu added a commit that referenced this issue Dec 12, 2023
Towards #6676  .

### Description

Adds a DDPM unet. 

Refactoring for some of the blocks here is scheduled
[here](#7227).

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [x] New tests added to cover the changes.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [x] In-line docstrings updated.
- [x] Documentation updated, tested `make html` command in the `docs/`
folder.

---------

Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
@vikashg
Copy link

vikashg commented Jan 4, 2024

Closing @marksgraham as it seems to be completed and merged.

@vikashg vikashg closed this as completed Jan 4, 2024
@marksgraham marksgraham reopened this Jan 4, 2024
@marksgraham
Copy link
Contributor Author

Hi @vikashg afraid this is still pending so re-opening!

@marksgraham marksgraham moved this from Done to Todo in MONAI v1.4 Jan 9, 2024
@KumoLiu
Copy link
Contributor

KumoLiu commented Jan 18, 2024

Add here as a reminder.
#7346 (comment)
#7346 (comment)

@vgrau98
Copy link
Contributor

vgrau98 commented Jan 24, 2024

I can start working on SABlock, integrating the private block and making refacto if needed if this is ok for you @marksgraham

@marksgraham
Copy link
Contributor Author

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.

marksgraham added a commit to marksgraham/MONAI that referenced this issue Jan 30, 2024
Partially fixes Project-MONAI#6676

### Description
Implements the AutoencoderKL network from MONAI Generative.

NB this network is subject to a planned refactor once the porting is
complete, [see
here](Project-MONAI#7227).

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [x] New tests added to cover the changes.
- [x] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [x] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [x] In-line docstrings updated.
- [x] Documentation updated, tested `make html` command in the `docs/`
folder.

---------

Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: Eric Kerfoot <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
marksgraham added a commit to marksgraham/MONAI that referenced this issue Jan 30, 2024
Towards Project-MONAI#6676  .

### Description

Adds a DDPM unet.

Refactoring for some of the blocks here is scheduled
[here](Project-MONAI#7227).

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [x] New tests added to cover the changes.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [x] In-line docstrings updated.
- [x] Documentation updated, tested `make html` command in the `docs/`
folder.

---------

Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
@marksgraham
Copy link
Contributor Author

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?

KumoLiu added a commit that referenced this issue Apr 23, 2024
Part of the refactoring in #7227 

### Description
Refactors autoencoderkl. Changes are:

- Introduce `CastToTempType` class for upsampling
- `Downsample` block removed and replaced by a `Sequential`
- The attention block now uses MONAI's `SABlock`, allowing a lot of code
to be removed
- Added a `load_old_state_dict` that allows for models trained on MONAI
Generative to be loaded in to this model, especially important given
some of the MONAI Generative's [model
zoo](https://github.com/Project-MONAI/GenerativeModels/tree/main/model-zoo)
uses this model. I have tested this works locally.

I discussed with @ericspod inheriting from `AutoEncoder` but after
experimentation have decided against it as it introduced changes that
made it very hard to ensure we could load model's trained in MONAI
Generative.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [ ] New tests added to cover the changes.
- [x] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

---------

Signed-off-by: KumoLiu <[email protected]>
Signed-off-by: kaibo <[email protected]>
Signed-off-by: heyufan1995 <[email protected]>
Signed-off-by: YunLiu <[email protected]>
Signed-off-by: binliu <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: axel.vlaminck <[email protected]>
Signed-off-by: monai-bot <[email protected]>
Signed-off-by: Ibrahim Hadzic <[email protected]>
Signed-off-by: Behrooz <[email protected]>
Signed-off-by: Timothy Baker <[email protected]>
Signed-off-by: Mathijs de Boer <[email protected]>
Signed-off-by: Fabian Klopfer <[email protected]>
Signed-off-by: Lucas Robinet <[email protected]>
Signed-off-by: Lucas Robinet <[email protected]>
Signed-off-by: chaoliu <[email protected]>
Signed-off-by: cxlcl <[email protected]>
Signed-off-by: chaoliu <[email protected]>
Signed-off-by: Suraj Pai <[email protected]>
Signed-off-by: Juan Pablo de la Cruz Gutiérrez <[email protected]>
Signed-off-by: elitap <[email protected]>
Signed-off-by: Felix Schnabel <[email protected]>
Signed-off-by: YanxuanLiu <[email protected]>
Signed-off-by: ytl0623 <[email protected]>
Signed-off-by: Dženan Zukić <[email protected]>
Signed-off-by: Ishan Dutta <[email protected]>
Signed-off-by: John Zielke <[email protected]>
Signed-off-by: Mingxin Zheng <[email protected]>
Signed-off-by: Vladimir Chernyi <[email protected]>
Signed-off-by: Yiheng Wang <[email protected]>
Signed-off-by: Szabolcs Botond Lorincz Molnar <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Signed-off-by: Lucas Robinet <[email protected]>
Signed-off-by: Mingxin <[email protected]>
Signed-off-by: Han Wang <[email protected]>
Signed-off-by: Konstantin Sukharev <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Co-authored-by: Kaibo Tang <[email protected]>
Co-authored-by: Yufan He <[email protected]>
Co-authored-by: binliunls <[email protected]>
Co-authored-by: Ben Murray <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Eric Kerfoot <[email protected]>
Co-authored-by: axel.vlaminck <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Mingxin Zheng <[email protected]>
Co-authored-by: monai-bot <[email protected]>
Co-authored-by: Ibrahim Hadzic <[email protected]>
Co-authored-by: Dr. Behrooz Hashemian <[email protected]>
Co-authored-by: Timothy J. Baker <[email protected]>
Co-authored-by: Mathijs de Boer <[email protected]>
Co-authored-by: Mathijs de Boer <[email protected]>
Co-authored-by: Fabian Klopfer <[email protected]>
Co-authored-by: Yiheng Wang <[email protected]>
Co-authored-by: Lucas Robinet <[email protected]>
Co-authored-by: Lucas Robinet <[email protected]>
Co-authored-by: cxlcl <[email protected]>
Co-authored-by: Suraj Pai <[email protected]>
Co-authored-by: Juampa <[email protected]>
Co-authored-by: elitap <[email protected]>
Co-authored-by: Felix Schnabel <[email protected]>
Co-authored-by: YanxuanLiu <[email protected]>
Co-authored-by: ytl0623 <[email protected]>
Co-authored-by: Dženan Zukić <[email protected]>
Co-authored-by: Ishan Dutta <[email protected]>
Co-authored-by: johnzielke <[email protected]>
Co-authored-by: Vladimir Chernyi <[email protected]>
Co-authored-by: Lőrincz-Molnár Szabolcs-Botond <[email protected]>
Co-authored-by: Nic Ma <[email protected]>
Co-authored-by: Lucas Robinet <[email protected]>
Co-authored-by: Han Wang <[email protected]>
Co-authored-by: Konstantin Sukharev <[email protected]>
KumoLiu added a commit that referenced this issue May 10, 2024
Part of #7227  .

### Description

A few sentences describing the changes proposed in this pull request.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [ ] New tests added to cover the changes.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

---------

Signed-off-by: KumoLiu <[email protected]>
Signed-off-by: kaibo <[email protected]>
Signed-off-by: heyufan1995 <[email protected]>
Signed-off-by: YunLiu <[email protected]>
Signed-off-by: binliu <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: axel.vlaminck <[email protected]>
Signed-off-by: monai-bot <[email protected]>
Signed-off-by: Ibrahim Hadzic <[email protected]>
Signed-off-by: Behrooz <[email protected]>
Signed-off-by: Timothy Baker <[email protected]>
Signed-off-by: Mathijs de Boer <[email protected]>
Signed-off-by: Fabian Klopfer <[email protected]>
Signed-off-by: Lucas Robinet <[email protected]>
Signed-off-by: Lucas Robinet <[email protected]>
Signed-off-by: chaoliu <[email protected]>
Signed-off-by: cxlcl <[email protected]>
Signed-off-by: chaoliu <[email protected]>
Signed-off-by: Suraj Pai <[email protected]>
Signed-off-by: Juan Pablo de la Cruz Gutiérrez <[email protected]>
Signed-off-by: elitap <[email protected]>
Signed-off-by: Felix Schnabel <[email protected]>
Signed-off-by: YanxuanLiu <[email protected]>
Signed-off-by: ytl0623 <[email protected]>
Signed-off-by: Dženan Zukić <[email protected]>
Signed-off-by: Ishan Dutta <[email protected]>
Signed-off-by: John Zielke <[email protected]>
Signed-off-by: Mingxin Zheng <[email protected]>
Signed-off-by: Vladimir Chernyi <[email protected]>
Signed-off-by: Yiheng Wang <[email protected]>
Signed-off-by: Szabolcs Botond Lorincz Molnar <[email protected]>
Signed-off-by: Lucas Robinet <[email protected]>
Signed-off-by: Mingxin <[email protected]>
Signed-off-by: Han Wang <[email protected]>
Signed-off-by: Konstantin Sukharev <[email protected]>
Signed-off-by: Ben Murray <[email protected]>
Signed-off-by: Matthew Vine <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Signed-off-by: Peter Kaplinsky <[email protected]>
Signed-off-by: Simon Jensen <[email protected]>
Signed-off-by: NabJa <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Co-authored-by: Kaibo Tang <[email protected]>
Co-authored-by: Yufan He <[email protected]>
Co-authored-by: binliunls <[email protected]>
Co-authored-by: Ben Murray <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Eric Kerfoot <[email protected]>
Co-authored-by: axel.vlaminck <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Mingxin Zheng <[email protected]>
Co-authored-by: monai-bot <[email protected]>
Co-authored-by: Ibrahim Hadzic <[email protected]>
Co-authored-by: Dr. Behrooz Hashemian <[email protected]>
Co-authored-by: Timothy J. Baker <[email protected]>
Co-authored-by: Mathijs de Boer <[email protected]>
Co-authored-by: Mathijs de Boer <[email protected]>
Co-authored-by: Fabian Klopfer <[email protected]>
Co-authored-by: Yiheng Wang <[email protected]>
Co-authored-by: Lucas Robinet <[email protected]>
Co-authored-by: Lucas Robinet <[email protected]>
Co-authored-by: cxlcl <[email protected]>
Co-authored-by: Suraj Pai <[email protected]>
Co-authored-by: Juampa <[email protected]>
Co-authored-by: elitap <[email protected]>
Co-authored-by: Felix Schnabel <[email protected]>
Co-authored-by: YanxuanLiu <[email protected]>
Co-authored-by: ytl0623 <[email protected]>
Co-authored-by: Dženan Zukić <[email protected]>
Co-authored-by: Ishan Dutta <[email protected]>
Co-authored-by: johnzielke <[email protected]>
Co-authored-by: Vladimir Chernyi <[email protected]>
Co-authored-by: Lőrincz-Molnár Szabolcs-Botond <[email protected]>
Co-authored-by: Nic Ma <[email protected]>
Co-authored-by: Lucas Robinet <[email protected]>
Co-authored-by: Han Wang <[email protected]>
Co-authored-by: Konstantin Sukharev <[email protected]>
Co-authored-by: Matthew Vine <[email protected]>
Co-authored-by: Pkaps25 <[email protected]>
Co-authored-by: Peter Kaplinsky <[email protected]>
Co-authored-by: Simon Jensen <[email protected]>
Co-authored-by: NabJa <[email protected]>
ericspod pushed a commit that referenced this issue May 13, 2024
Part of #7227  .

### Description

A few sentences describing the changes proposed in this pull request.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [ ] New tests added to cover the changes.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

Signed-off-by: Mark Graham <[email protected]>
marksgraham added a commit that referenced this issue May 14, 2024
Towards #7227  .

### Description
There were lots of contigous calls in the DiffusionModelUnet. It turns
out these are necessary after attention blocks, as the einops operation
sometimes leads to non-contigous tensors that can cause errors. I've
tidied the code up so the .contiguous calls are only after attention
calls.

A few sentences describing the changes proposed in this pull request.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [ ] New tests added to cover the changes.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

---------

Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: Eric Kerfoot <[email protected]>
KumoLiu added a commit that referenced this issue May 22, 2024
Part of #7227  .

### Description
Tidies up some of controlnet

A few sentences describing the changes proposed in this pull request.

### Types of changes
<!--- Put an `x` in all the boxes that apply, and remove the not
applicable items -->
- [x] Non-breaking change (fix or new feature that would not break
existing functionality).
- [ ] Breaking change (fix or new feature that would cause existing
functionality to change).
- [ ] New tests added to cover the changes.
- [ ] Integration tests passed locally by running `./runtests.sh -f -u
--net --coverage`.
- [ ] Quick tests passed locally by running `./runtests.sh --quick
--unittests --disttests`.
- [ ] In-line docstrings updated.
- [ ] Documentation updated, tested `make html` command in the `docs/`
folder.

---------

Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
@ericspod
Copy link
Member

ericspod commented Aug 1, 2024

@virginiafdez we should come back to the last two items here to see if we want to address anything next.

@KumoLiu
Copy link
Contributor

KumoLiu commented Aug 1, 2024

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.

self.qkv = nn.Linear(self.hidden_input_size, self.inner_dim * 3, bias=qkv_bias)

https://github.com/Project-MONAI/GenerativeModels/blob/7428fce193771e9564f29b91d29e523dd1b6b4cd/generative/networks/blocks/selfattention.py#L80-L83

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?

@KumoLiu KumoLiu removed this from MONAI v1.4 Sep 20, 2024
@KumoLiu KumoLiu moved this to In progress in MONAI v1.5 Sep 20, 2024
@KumoLiu KumoLiu moved this from In progress to Backlog in MONAI v1.5 Sep 20, 2024
@KumoLiu KumoLiu moved this from Backlog to In progress in MONAI v1.5 Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor Non-breaking feature enhancements
Projects
Status: In progress
Development

No branches or pull requests

6 participants