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 inferers #7379

Merged

Conversation

marksgraham
Copy link
Contributor

@marksgraham marksgraham commented Jan 9, 2024

Part of #6676 .

Description

Adds Inferers to assist with training and sampling from diffusion models and controllers.

Also takes the opportunity to make two changes which slipped through the previous PRs:

  • rename the num_channels arg in the spade diffusion unet to channels to be consistent with all the other models added from Generative - this slipped through in the networks PR.
  • add the Ordering class to __init__.py for easier import

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.

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.
Looks like we introduce one inferer for one model, can we refactor them such as make DiffusionInferer more general if comes into other model. Perhaps accept pre-process or post-process or same thing others.

monai/inferers/inferer.py Outdated Show resolved Hide resolved
monai/inferers/inferer.py Outdated Show resolved Hide resolved
monai/inferers/inferer.py Outdated Show resolved Hide resolved
monai/inferers/inferer.py Show resolved Hide resolved
monai/inferers/inferer.py Show resolved Hide resolved
marksgraham and others added 8 commits January 10, 2024 11:57
Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
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.

@ericspod, could you please also help review this PR?
Thanks!

monai/inferers/inferer.py Outdated Show resolved Hide resolved
monai/inferers/inferer.py Outdated Show resolved Hide resolved
monai/inferers/inferer.py Outdated Show resolved Hide resolved
monai/inferers/inferer.py Outdated Show resolved Hide resolved
monai/inferers/inferer.py Show resolved Hide resolved
monai/inferers/inferer.py Outdated Show resolved Hide resolved
monai/inferers/inferer.py Show resolved Hide resolved
monai/inferers/inferer.py Outdated Show resolved Hide resolved
monai/inferers/inferer.py Show resolved Hide resolved
monai/inferers/inferer.py Outdated Show resolved Hide resolved
marksgraham and others added 4 commits January 11, 2024 14:28
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
marksgraham and others added 8 commits January 11, 2024 14:30
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Co-authored-by: YunLiu <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
I, Mark Graham <[email protected]>, hereby add my Signed-off-by to this commit: 553c94b

Signed-off-by: Mark Graham <[email protected]>
Signed-off-by: Mark Graham <[email protected]>
@KumoLiu
Copy link
Contributor

KumoLiu commented Jan 16, 2024

/build

@KumoLiu KumoLiu enabled auto-merge (squash) January 16, 2024 07:21
@KumoLiu
Copy link
Contributor

KumoLiu commented Jan 16, 2024

Hi @marksgraham, could you please check-pick the commit here after it merged which caused a CI error and could not merge?
Thanks.

@KumoLiu
Copy link
Contributor

KumoLiu commented Jan 16, 2024

/build

@marksgraham
Copy link
Contributor Author

Hi @marksgraham, could you please check-pick the commit here after it merged which caused a CI error and could not merge? Thanks.

Hi,
There is a test in test_ordering that sometimes fails as it has random behaviour. I don't think the test is useful and should be removed. Shall I remove it on this branch before we merge?

@KumoLiu
Copy link
Contributor

KumoLiu commented Jan 16, 2024

I think the error is caused by the update from flake8-bugbear instead of test_ordering. But anyway it is also not related to the PR, you could pin the version to avoid the error first, I will try to create a PR to fix it in the dev branch.
https://github.com/Project-MONAI/MONAI/actions/runs/7544331654/job/20537411217#step:2:2601

@KumoLiu
Copy link
Contributor

KumoLiu commented Jan 18, 2024

/build

@KumoLiu
Copy link
Contributor

KumoLiu commented Jan 18, 2024

Hi @marksgraham, now the error seems related to the test_ordering.py. As it is random, can we add set_determinism to ensure the test passes?
Something like:

set_determinism(seed=0)

def set_determinism(

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

hi @KumoLiu

I have removed the test. As far as I can tell, the test does not make sense and does not seem to be usefully testing any part of the ordering code.

@KumoLiu
Copy link
Contributor

KumoLiu commented Jan 18, 2024

/build

@KumoLiu KumoLiu merged commit 510f7bc into Project-MONAI:gen-ai-dev Jan 18, 2024
28 checks passed
marksgraham added a commit to marksgraham/MONAI that referenced this pull request Jan 30, 2024
Part of Project-MONAI#6676  .

### Description

Adds Inferers to assist with training and sampling from diffusion models
and controllers.

Also takes the opportunity to make two changes which slipped through the
previous PRs:
- rename the `num_channels` arg in the spade diffusion unet to
`channels` to be consistent with all the other models added from
Generative - this slipped through in the networks PR.
- add the `Ordering` class to `__init__.py` for easier import

### 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]>
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