feat: expose training presets to enable high level API#128
Conversation
There was a problem hiding this comment.
I like the new setup; it is reducing boilerplate a lot.
The only concern is that we could end up with two sources of truth: one where we provide the defaults via experiment YAMLS/configs, and one where we hardcode it in those presets. Ideally, we have one (or one that relies on the other).
Moreover, for every new model/recipe/etc, what we will add in the future, do we have to provide those presets as well?
Finally, the defaults have quite some overlap with the templates that will be provided with noether-init, how do we solve the problem that we copy around too much stuff?
HennerM
left a comment
HennerM
left a comment
There was a problem hiding this comment.
Providing sensible defaults is super useful. The presets are useful as well but I worry that there is a lot of overlap with the tutorial and potentially recipes. I think if we could get it down to one or two preset that cover all all the aerodynamics datasets this would be a bit more managable.
| self, | ||
| *, | ||
| model_kind: str, | ||
| model_params: dict[str, Any] | None = None, |
There was a problem hiding this comment.
I think it would be nice to have the actual type here, to keep the benefit of the Python types
Ndles
force-pushed
the
feat/simplify-training-interface
branch
from
d6a6584 to
1dd4e1d
Compare
|
Ndles
force-pushed
the
feat/simplify-training-interface
branch
from
76857df to
9a7b38f
Compare
| LOSS_REGISTRY: dict[str, Callable[..., torch.Tensor]] = { | ||
| "mse": F.mse_loss, | ||
| "l1": F.l1_loss, | ||
| "smooth_l1": F.smooth_l1_loss, | ||
| "huber": F.huber_loss, | ||
| } |
There was a problem hiding this comment.
IMO we can avoid needing a registry if we just let the user specify the class path for one of the torch.nn.modules.loss classes, such as torch.nn.modules.loss.L1Loss.
There was a problem hiding this comment.
yeah I wasn't a huge fan of that long string as an argument. Forces people to learn exact import names. But we can address it in the followup as well. I am not biased on that.
| from noether.core.presets import DomainPreset | ||
| from noether.core.schemas.dataset import DatasetBaseConfig, DatasetWrappers | ||
|
|
||
| AERO_PIPELINE = "tutorial.pipeline.AeroMultistagePipeline" |
There was a problem hiding this comment.
should we move the aero pipeline also into Noether or do you think it's okay to keep in the tutorial?
There was a problem hiding this comment.
I think it's an open discussion point for the followup. I think it makes sense to make it part of the shared recipes, but maybe making it available in the noether itself is not a bad idea.
3 participants
DomainPresetbase class (src/noether/core/presets/) that encapsulates dataset-specific configuration (stats, normalizers, pipeline defaults, forward properties) so training scripts only specify experiment-level knobs (model kind, hidden_dim, epochs)AeroCFDPresetintermediate class and 5 concrete domain presets (AhmedML, DrivAerML, DrivAerNet (will add in the next commits), EmmiWing (will add in the next commits), ShapeNetCar) inexamples/presets/ABUPTWrapper,UPTWrapper,TransformerWrapper,TransolverWrapperinsrc/noether/modeling/models/wrappers/model_defaults.pyregistry that derives sub-configs (transformer_block_config, supernode_pooling_config, decoder_config) from user-facing knobs like hidden_dim and num_headsexamples/train_*.py) with one function per model, exposing device, accelerator, dataset_root, output_path as keyword argsWeightedMSETrainertonoether.training.trainersexports