Add RatioEstimatorBuilder for NRE trainers and base class improvements#1920
Conversation
d442dce to
8d5def0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## gsoc-2026 #1920 +/- ##
=============================================
- Coverage 87.99% 87.98% -0.02%
=============================================
Files 144 144
Lines 13499 13717 +218
=============================================
+ Hits 11879 12069 +190
- Misses 1620 1648 +28
Flags with carried forward coverage won't be shown. Click here to find out more.
|
janfb
left a comment
There was a problem hiding this comment.
Thanks @satwiksps, overall very clean extension of the builder pattern to all four NRE trainers. great that you added the role-shape regression test (test_builder_role_shapes).
As discussed in our meeting I made several longer comments that go beyond the NRE code, mostly improvements that belong in the base class, and although this is a bit of scope creep I think this PR is the right place. It will make the the upcoming VF builder and other refactorings much easier.
Let me know if anything is unclear!
| embedding_net_x: Optional[Any] = None | ||
| embedding_net_y: Optional[Any] = None |
There was a problem hiding this comment.
the type here could be narrower, e.g., nn.Module as this should always be a NN. At some point we considered allowing Callables that return a nn.Module, but that path is actually not implemented, so no need to type-support it for now. better be more narrow.
this also applies to the other *Builder subclasses.
I also checked those classes for possibly more narrower types and there are a couple of candidates (please check as well), e.g.,:
MixedDensityEstimatorBuilder.num_categories_per_variable(:386): downstream is Optional[Tensor] → narrow fromAnytoOptional[Tensor].RatioEstimatorBuilder.norm_layer(:484, already Optional[Callable]): downstream isCallable[[int], nn.Module](defaultnn.LayerNorm) → tighten to
Optional[Callable[[int], nn.Module]].
| _VALID_CLASSIFIER_MODELS = frozenset(get_args(CLASSIFIER_MODELS)) | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
When this gets changed to repr=False from the comment above, you would need to add one custom __repr__ on the base so configs read the way users wrote them. At the moment, repr(RatioEstimatorBuilder(model="resnet", hidden_features=64)) prints all 11 fields starting with the internal extra_kwargs={} and nine Nones, potentially hiding the two the user set. A base __repr__ that leads with model and omits unset/default fields yields RatioEstimatorBuilder(model='resnet', hidden_features=64). (Lives in the base, so all builders benefit.)
There was a problem hiding this comment.
When building the repr note that it's important to
- Always show the discriminator (
model/continuous_model), even when it equals its default, otherwiseDensityEstimatorBuilder(model="maf")(maf is the default) reprs asDensityEstimatorBuilder(), hiding the one field that matters. - Skip empty
extra_kwargs. It usesdefault_factory, sofield.defaultisMISSING, not{}; a naivevalue == field.defaultcheck won't drop it and you get a strayextra_kwargs={}.
75f7a74 to
a7c12dd
Compare
…idation * Converted all builder classes to use @DataClass(frozen=True, eq=False, repr=False) to ensure immutability. * Added _reject_inapplicable_fields() to fail-fast and throw an error if a user passes hyperparameters that the chosen model's build function would silently ignore. * Implemented a custom __repr__ that hides empty or default fields but always displays the discriminator field (like model or continuous_model). * Replaced the None default for extra_kwargs with ield(default_factory=dict) to handle arbitrary kwargs safely. * Renamed the z-score fields to z_score_input and z_score_condition, while using a _BUILD_KWARG_ALIASES map to maintain compatibility with downstream build functions. * Added _literal_values() to catch typos in Literal fields immediately when the class is constructed. * Tightened the type annotations for fields like embedding_net, um_categories_per_variable, and orm_layer. * Moved the dispatch dictionaries out to module-level _density_build_fns() and _classifier_build_fns() helpers to keep the code clean and prevent circular imports.
a7c12dd to
8122159
Compare
janfb
left a comment
There was a problem hiding this comment.
Looking mostly done, great! 👏
Just one missed argument and a couple of smaller improvement comments.
What does this PR do?
This PR continues the GSoC 2026 Neural Network Builder API refactor by introducing the
RatioEstimatorBuilderfor all four NRE trainers (NRE_A,NRE_B,NRE_C,BNRE).Instead of passing a string like
classifier="resnet"and hiding settings in kwargs, we can now pass a fully typed and clear configuration likeRatioEstimatorBuilder(model="resnet", hidden_features=64)to use linear, MLP, or ResNet models. PassingNonewill silently default to a ResNet builder to keep the old behavior, while using the old string arguments is now deprecated.Files Changed
estimator_configs.pyRatioEstimatorBuilderdataclass configures and validates linear/MLP/ResNet classifiers, withbuild()routing to the right function.dataclasses.replace()to tweak), with saferextra_kwargshandling and a cleaner__repr__that only shows what's actually set.z_score_x/ytoz_score_input/conditionfor clarity, with aliasing so it still maps correctly downstream.Literalvalues and flags hyperparameters that don't apply to the chosen model.nre_base.pyclassifiernow takes aRatioEstimatorBuilder(defaults toNone-> ResNet); old string configs still work but warn, wrong types fail fast._wrap_builderto bridge the new builder API to the legacy(batch_theta, batch_x)format.nre_a.py,nre_b.py,nre_c.py,bnre.pyclassifiertype hint/default update, docstrings refreshed to match.npe_base.py,nle_base.py,mnle.py,mnpe.pymnle.py/mnpe.py.sbi/neural_nets/__init__.pyDensityEstimatorBuilderandMixedDensityEstimatorBuilderso those import paths actually resolve.density_estimator_builder_test.pyz_score_input/z_score_conditionrename.nre_builder_integration_test.py(new)npe_nle_builder_integration_test.pyAI Usage
Does this close any issues?
N/A
Any relevant code examples, logs, or error messages?
N/A