physicsnemo icon indicating copy to clipboard operation
physicsnemo copied to clipboard

[CorrDiff]: refactor corrdiff configs to dataclasses

Open nbren12 opened this issue 1 year ago • 5 comments

          @mnabian Can we make an issue to refactor all these options to a common dataclass, which we can then instantiate with https://hydra.cc/docs/1.0/tutorials/structured_config/minimal_example/. Using a dataclass will allow automatic completion, a single point for documentation, and more generally facilitate validation/static checking.

Originally posted by @nbren12 in https://github.com/NVIDIA/modulus/pull/401#discussion_r1537923976

nbren12 avatar Apr 04 '24 20:04 nbren12

@mnabian I could make this refactor if you like. Thoughts?

For example, this code:

    # Parse deterministic sampler options
    sigma_min = getattr(cfg, "sigma_min", None)
    sigma_max = getattr(cfg, "sigma_max", None)
    rho = getattr(cfg, "rho", 7)
    solver = getattr(cfg, "solver", "heun")
    discretization = getattr(cfg, "discretization", "edm")
    schedule = getattr(cfg, "schedule", "linear")
    scaling = getattr(cfg, "scaling", None)
    S_churn = getattr(cfg, "S_churn", 0)
    S_min = getattr(cfg, "S_min", 0)
    S_max = getattr(cfg, "S_max", float("inf"))
    S_noise = getattr(cfg, "S_noise", 1)

would change to

@dataclass
class SamplerConfig:
    sigma_min: Optional[float] = None
    sigma_max: Optional[float] = None
    rho: int = 7
    solver: str = "heun"
    discretization: str = "edm"
    schedule: str = "linear"
    scaling: Optional[float] = None
    S_churn: int = 0
    S_min: int = 0
    S_max: float = float("inf")
    S_noise: int = 1
```

nbren12 avatar Apr 04 '24 20:04 nbren12

Yes I think this will clean up the configs. Please go ahead and let me know if I can help.

mnabian avatar Apr 05 '24 00:04 mnabian

@nbren12 is this still WIP? :) I'd volunteer take it off your plate if not.

DavidLandup0 avatar May 06 '24 04:05 DavidLandup0

Go for it @DavidLandup0 !

nbren12 avatar May 08 '24 23:05 nbren12

@nbren12 thanks! I'll open a PR later today or tomorrow :)

DavidLandup0 avatar May 09 '24 07:05 DavidLandup0

@mnabian this overlaps with 824. I already started to work on that so I'll reassign myself.

CharlelieLrt avatar Feb 05 '25 02:02 CharlelieLrt