Refactor corrector configs to the Builder pattern#1313
Conversation
Each *CorrectorConfig now builds its corrector via a _build() helper that constructs sub-objects from sub-config fields and packages remaining scalars into a plain (non-dacite) leaf params dataclass, which it passes to the impl. The corrector impls no longer read self._config, removing the Builder-pattern violation where a non-leaf config reached the implementation class. - fme.core.corrector.atmosphere: add AtmosphereCorrectorParams; AtmosphereCorrector takes params instead of the config; energy-budget sub-config flattened to scalars. - fme.core.corrector.ocean: add OceanCorrectorParams; OceanCorrector takes params plus the sea_ice_fraction_correction sub-object; flux/OHC sub-configs flattened to scalars. - fme.core.corrector.ice: IceCorrector takes the budget_correction sub-object directly. - Route the corrector unit tests through config._build(...) so they exercise the real build path; behavior-preserving (all corrector tests pass).
b54ee37 to
54857d0
Compare
| self, | ||
| config: OceanCorrectorConfig, | ||
| params: OceanCorrectorParams, | ||
| sea_ice_fraction_correction: SeaIceFractionConfig | None, |
There was a problem hiding this comment.
This kind of begs the question of why the other corrections don't also get passed by having Callables passed as argument instead of having the code in the corrector and the options on params, but that refactor is out-of-scope for this PR.
There was a problem hiding this comment.
I recall there being some emphasis in the past on having the config classes contain their associated ops as methods, but agreed it would be good to have this be more consistent.
There was a problem hiding this comment.
Would it be better to pass SeaIceFractionConfig.__call__ via corrector params?
There was a problem hiding this comment.
I think it would be better to pass the .__call__ than the config, it did confuse me to see a Config being passed and later realize it's a Callable.
I'm not sure about passing it on the params though - params feels like it should be a container of primitive could-be-yaml-configurable types (even if the yaml is massaged into this type). Without that boundary, you have to also ask whether the gridded_operations should be on the params, for example. It's kind of nice for the explicit arguments to describe all of the custom types/complex objects/callables this type relies on, and have params sweep under the rug all the types that don't have code related to it, and are just configurables. That way yaml answers "how is this object configurable" while the other arguments answer "what operations/operators and complex data types does this code rely on"
There was a problem hiding this comment.
Maybe we can refactor to pass the Callable, and make a follow-on PR right now based from this branch to refactor the other corrections to behave more like the SeaIceFractionConfig and also pass Callables.
jpdunc23
left a comment
There was a problem hiding this comment.
I think it is a good idea to avoid the many opaque / potentially brittle self._config.* reads in the corrector.
Disallowing "a non-leaf config reaching its implementation class" is probably a good rule-of-thumb, but IMO this PR reshuffles things without improving the situation: We're simply repeating all of the leaf corrector config attributes on new non-config dataclass like *CorrectorParams, and this doesn't change what I see as the more fundamental problem here of having the *Corrector implementation know too much about the leaf configs.
I think a better approach is to follow the SeaIceFractionConfig pattern where the leaf config class has a method (__call__ or something else) that accesses its own config attributes so that the corrector implementation doesn't have to worry about those low-level details. An object like *CorrectorParams (perhaps *CorrectorCalls is a better name for what I'm proposing) can then simply be a collection of these methods (including, perhaps, "no-op" methods for cases where the leaf config is null), and all the *Corrector needs to know is in what order to call the *CorrectorCalls items.
| self, | ||
| config: OceanCorrectorConfig, | ||
| params: OceanCorrectorParams, | ||
| sea_ice_fraction_correction: SeaIceFractionConfig | None, |
There was a problem hiding this comment.
I recall there being some emphasis in the past on having the config classes contain their associated ops as methods, but agreed it would be good to have this be more consistent.
| self, | ||
| config: OceanCorrectorConfig, | ||
| params: OceanCorrectorParams, | ||
| sea_ice_fraction_correction: SeaIceFractionConfig | None, |
There was a problem hiding this comment.
Would it be better to pass SeaIceFractionConfig.__call__ via corrector params?
First wave of the Builder-pattern burn-down (goal: ace obeys the Builder-pattern pass). The
fme/core/corrector/configs are non-leaf (they hold sub-configs), yet each corrector impl read manyself._config.*fields — the violation the pass targets: a non-leaf config reaching its implementation class. This refactors the three correctors so each*CorrectorConfigconstructs the corrector via a_build()helper that hands the impl pre-built sub-objects plus a plain (non-dacite) leaf params dataclass for the scalars, and the impls stop reading the config. Behavior-preserving — all corrector unit tests pass unchanged in intent. Intended as the template-setting sibling for the rest of the burn-down.Changes:
fme.core.corrector.atmosphere: addAtmosphereCorrectorParams;AtmosphereCorrector.__init__takesparamsinstead of the config;_get_correctordelegates to_build(...), which flattens theEnergyBudgetConfigsub-config to scalar params (total_energy_budget_methodisNoneiff disabled).fme.core.corrector.ocean: addOceanCorrectorParams;OceanCorrector.__init__takesparamsplus thesea_ice_fraction_correctioncallable sub-object;_build(...)flattens the surface-energy-flux and ocean-heat-content sub-configs to scalar params.fme.core.corrector.ice:IceCorrector.__init__takes thebudget_correctionsub-object directly (no params dataclass — no loose scalars);_build(...)added.test_atmosphere,test_ocean: route direct impl construction throughconfig._build(...)so the tests exercise the real build path; drop the now-unused impl imports.Tests added (existing corrector tests re-routed through the build path; no new behavior to cover)
If dependencies changed, "deps only" image rebuilt and "latest_deps_only_image.txt" file updated