Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the FunctionalCPD implementation by extracting the fitting and representation logic into new TabularAdapter and LinearGaussianAdapter classes. This improves code modularity and simplifies the FunctionalCPD class. The review feedback identifies several opportunities to enhance the robustness of these new adapters, including handling potential division-by-zero errors in tabular data processing, preventing NaN propagation when dealing with empty datasets in linear models, and ensuring consistent estimator type validation across different adapter implementations.
| variable_states = sorted(data[self.variable].dropna().unique()) | ||
| if not self.parents: | ||
| counts = data[self.variable].value_counts().reindex(variable_states, fill_value=0) | ||
| probs = counts.values / counts.values.sum() |
There was a problem hiding this comment.
Potential division by zero if counts.values.sum() is 0. This can occur if the input data for the variable is empty or contains only NaNs after dropna().
| probs = counts.values / counts.values.sum() | |
| probs = counts.values / (counts.values.sum() or 1) |
| self.parents = parents if parents is not None else [] | ||
|
|
||
| def fit(self, data): | ||
| if self.estimator not in ("MLE", "OLS", None): |
There was a problem hiding this comment.
The estimator check is inconsistent with TabularAdapter. It should also allow the MLE class for consistency, as it is imported in this file.
| if self.estimator not in ("MLE", "OLS", None): | |
| if self.estimator not in ("MLE", "OLS", MLE, None): |
| variance = residuals[0] / len(target_data) | ||
| else: | ||
| predictions = design_matrix @ beta | ||
| variance = np.mean((target_data - predictions) ** 2) |
There was a problem hiding this comment.
np.mean on an empty array returns NaN, which will result in std being NaN if target_data is empty. It's safer to handle the empty case explicitly to avoid propagating NaN values into the CPD.
| variance = np.mean((target_data - predictions) ** 2) | |
| variance = np.mean((target_data - predictions) ** 2) if len(target_data) > 0 else 0.0 |
1 participant
Motivation
FunctionalCPD_Refactorcontained growing fitting and representation logic for tabular and linear tags that obscured responsibilities and increased class size.SkproAdapterpattern and making__repr__logic reusable.Description
pgmpy/factors/hybrid/Adapters.pywithTabularAdapterandLinearGaussianAdapterto handle fitting and__repr__for tabular and linear CPDs respectively.pgmpy/factors/hybrid/FunctionalCPD_Refactor.pyto delegate_fit_tabularand_fit_linearto the new adapters and to store the adapter instance onadapter_.SkproAdapterinstance onadapter_and setfitted_cpd_accordingly for consistency.FunctionalCPD_Refactor.__repr__to reuse adapter__repr__for the tabular and linear cases and kept a generic fitted/unfitted representation otherwise.Testing
pytest -v pgmpy/tests/test_models/test_FunctionalBayesianNetwork_Refactor.pyand the suite produced3 passed, 1 skipped.pre-commit run --all-filesbut it could not be executed in the environment becausepre-commitis not installed.Codex Task