Restructure diffusion subpackage #1268
Conversation
No tests fixed yet.
phsyicsnemo.utils, launch.config is just gone. It was empty.
CharlelieLrt
removed request for
Alexey-Kamenev,
NickGeneva,
RishikeshRanade,
dallasfoster,
laserkelvin,
mnabian and
peterdsharpe
CharlelieLrt
added
3 - Ready for Review
CharlelieLrt
changed the title
Signed-off-by: Charlelie Laurent <[email protected]>
Signed-off-by: Charlelie Laurent <[email protected]>
…t exists anymore Signed-off-by: Charlelie Laurent <[email protected]>
Signed-off-by: Charlelie Laurent <[email protected]>
| # from .YParams import YParams | ||
| # from .dataset import Era5Dataset, CWBDataset, CWBERA5DatasetV2, ZarrDataset | ||
|
|
||
| # ---------------------------------------------------------------------------- |
There was a problem hiding this comment.
Did this somehow get removed from Corey's v2.0 PR, and you are reintroducing it here?
There was a problem hiding this comment.
The git merge was an absolute mess, so it's very possible I made mistake...
But after merging with Corey's v2.0PR what I got here was an empty file with an unused import os. I assume Corey just forgot to remove it? So I deleted the unused import
@coreyjadams for viz
There was a problem hiding this comment.
Well I don't see a physicsnemo.compat in main branch at the moment so I think it was somehow dropped from the big v2.0 PR?
There was a problem hiding this comment.
Yeah my rebase onto main after the merge was also a disaster. I think squashing commits makes it impossible for git to actually merge properly, and we had too many squashed?
I had removed compat from v2.0 - I thought it was breaking doc tests (I no longer think this) but I also thought, "is this really useful?". Anyways it sounds like there is more will to reintroduce it, I won't stand in the way.
There was a problem hiding this comment.
I think it's def useful as it'll be another tool in our belt for helping people migrate
There was a problem hiding this comment.
OK, I'll bring it back in a standalone PR. @CharlelieLrt is innocent in this one!
| class Attention(torch.nn.Module): | ||
| """ | ||
| Self-attention block used in U-Net-style architectures, such as DDPM++, NCSN++, and ADM. | ||
| Applies GroupNorm followed by multi-head self-attention and a projection layer. |
There was a problem hiding this comment.
I don't love having the module named nn.attention_layers.Attention be this relatively model-specific U-Net version -- seems like such a module should be relatively general. Thoughts on renaming it?
I still have mixed feelings about the 'spell out any number less than 10', but it is the style guide rule that I am contractually required to make. The only way around it is for it to be in code font....
I want to init-cap all the 'utils' in the headings.....but it could be considered a code thing, so I am leaving it.
|
I had small changes to the two rst files. If those get merged well, I approve. |
Signed-off-by: Charlelie Laurent <[email protected]>
Signed-off-by: Charlelie Laurent <[email protected]>
Signed-off-by: Charlelie Laurent <[email protected]>
…/CharlelieLrt/modulus into restructure-diffusion-subpackage
6 participants
PhysicsNeMo Pull Request
Description
Restructure Diffusion Subpackage
This PR establishes the foundational structure for the
physicsnemo.diffusionsubpackage, organizing diffusion-related functionality into a clean, layered architecture.Summary
Created
physicsnemo.diffusionpackage with dedicated submodules:generate/- Higher-level generation utilities and pipelinessamplers/- Samplers based on ODE and SDE solversmetrics/- Loss functions and diffusion-specific metrics (FID, etc.)preconditioners/- Preconditioning model wrappersmulti_diffusion/- Patch-based diffusion utilitiesdenoisers/- Placeholder for future denoiser implementations; to be used in samplersnoise_schedulers/- Placeholder for future noise scheduler implementationsutils/- Shared utilitiesAdded warning system for incremental migration:
FutureFeatureWarningfor placeholder modules not yet implementedLegacyFeatureWarningfor existing code that will be deprecated in future releasesReorganized UNet models:
SongUNetmodels and application-specific UNet wrappers tophysicsnemo.models.diffusion_unetsExtracted reusable layers into
physicsnemo.nn:This includes for example:
group_norm.py- GroupNorm variants with optional FP32 conversionunet_layers.py- UNet encoder/decoder blocks, attention blocksembedding_layers.py- Positional and Fourier embeddingsEtc...
Cleaned up legacy code:
physicsnemo.models.diffusion/examples/directoryAdded import-linter contracts for the diffusion package:
generate→samplers/metrics→preconditioners/multi_diffusion/denoisers/noise_schedulers→utilsUpdated all examples and tests to use the new package structure
I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.
The CHANGELOG.md is up to date with these changes.
An issue is linked to this pull request.
Dependencies
Review Process
All PRs are reviewed by the PhysicsNeMo team before merging.
Depending on which files are changed, GitHub may automatically assign a maintainer for review.
We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.
AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.