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

6676 port generative networks spade #7320

Merged

Conversation

marksgraham
Copy link
Contributor

@marksgraham marksgraham commented Dec 14, 2023

Towards #6676 .

Description

This adds SPADE-enabled autoencoder and diffusion_model_unet architectures. They are new implementations for each network, rather than options in the existing network, because @virginiafdez and I felt that adding additional options to the existing networks to enable spade compatibility significantly reduced the readability of them for users who were not interested in SPADE functionality.

These are the last networks to be ported over.

Types of changes

  • 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 <markgraham539@gmail.com>
Signed-off-by: Mark Graham <markgraham539@gmail.com>
Signed-off-by: Mark Graham <markgraham539@gmail.com>
Copy link
Member

@ericspod ericspod left a comment

Choose a reason for hiding this comment

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

Looks good for merging into the branch. We can just keep track of the things I mentioned.

Copy link
Contributor

@KumoLiu KumoLiu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and hard work!
I want to confirmed that the refactor will also be done in this branch, as previously discussed, right?

docs/source/networks.rst Outdated Show resolved Hide resolved
monai/networks/blocks/spade_norm.py Outdated Show resolved Hide resolved
monai/networks/blocks/spade_norm.py Show resolved Hide resolved
monai/networks/blocks/spade_norm.py Outdated Show resolved Hide resolved
monai/networks/nets/spade_autoencoderkl.py Show resolved Hide resolved
monai/networks/nets/spade_diffusion_model_unet.py Outdated Show resolved Hide resolved
@ericspod
Copy link
Member

Thanks for the PR and hard work!
I want to confirmed that the refactor will also be done in this branch, as previously discussed, right?

We definitely will be refactoring a lot of things in this feature branch before merging into dev. There's a lot of structural overlap between networks and redundant block definitions we can consolidate. We'll also do a pass over documentation to make sure it's consistent and complete.

marksgraham and others added 8 commits December 18, 2023 14:59
Co-authored-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Signed-off-by: Mark Graham <markgraham539@gmail.com>
Signed-off-by: Mark Graham <mark@Marks-MacBook-Pro.local>
Signed-off-by: Mark Graham <markgraham539@gmail.com>
Signed-off-by: Mark Graham <markgraham539@gmail.com>
…sgraham/MONAI into 6676_port_generative_networks_spade
Co-authored-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Signed-off-by: Mark Graham <markgraham539@gmail.com>
Signed-off-by: Mark Graham <markgraham539@gmail.com>
marksgraham and others added 2 commits December 18, 2023 15:34
I, Mark Graham <markgraham539@gmail.com>, hereby add my Signed-off-by to this commit: 171fec8

Signed-off-by: Mark Graham <markgraham539@gmail.com>
@marksgraham
Copy link
Contributor Author

marksgraham commented Dec 18, 2023

Hi @KumoLiu
Thanks for the feedback - I've addressed the minor bits and added the others to the refactoring issue, #7227.

Just a few more components to add (noise schedulers for diffusion models, some Inferers and Engines) and then we can begin the refactor. And to echo Eric, yes, we will do the refactor on this branch.

@KumoLiu
Copy link
Contributor

KumoLiu commented Dec 18, 2023

/build

1 similar comment
@KumoLiu
Copy link
Contributor

KumoLiu commented Dec 19, 2023

/build

@KumoLiu KumoLiu merged commit aa4a4db into Project-MONAI:gen-ai-dev Dec 19, 2023
28 checks passed
marksgraham added a commit to marksgraham/MONAI that referenced this pull request Jan 30, 2024
Towards Project-MONAI#6676  .

### Description

This adds SPADE-enabled autoencoder and diffusion_model_unet
architectures. They are new implementations for each network, rather
than options in the existing network, because @virginiafdez and I felt
that adding additional options to the existing networks to enable spade
compatibility significantly reduced the readability of them for users
who were not interested in SPADE functionality.

These are the last networks to be ported over.

### 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 <markgraham539@gmail.com>
Signed-off-by: Mark Graham <mark@Marks-MacBook-Pro.local>
Co-authored-by: YunLiu <55491388+KumoLiu@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: Mark Graham <markgraham539@gmail.com>
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