[ENH] refactor metric classes to one class per file#1035
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors the skpro.metrics module by extracting individual metric classes into separate private modules and updating the package exports accordingly, replacing the previous monolithic _classes.py.
Changes:
- Split metric implementations into one-class-per-file private modules (e.g.,
_pinball.py,_logloss.py,_crps.py) - Removed the old aggregated
skpro/metrics/_classes.py - Updated
skpro/metrics/__init__.pyto import metrics from the new modules
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| skpro/metrics/_squared_loss.py | New module containing SquaredDistrLoss extracted from the old _classes.py. |
| skpro/metrics/_pinball.py | New module containing PinballLoss extracted from the old _classes.py. |
| skpro/metrics/_logloss_linearlized.py | New module containing LinearizedLogLoss extracted from the old _classes.py (but with spelling issues). |
| skpro/metrics/_logloss.py | New module containing LogLoss extracted from the old _classes.py. |
| skpro/metrics/_interval_width.py | New module containing IntervalWidth extracted from the old _classes.py. |
| skpro/metrics/_empirical_coverage.py | New module containing EmpiricalCoverage extracted from the old _classes.py. |
| skpro/metrics/_crps.py | New module containing CRPS extracted from the old _classes.py. |
| skpro/metrics/_constraint_violation.py | New module containing ConstraintViolation extracted from the old _classes.py. |
| skpro/metrics/_classes.py | Removed the previous single-file container of metric classes. |
| skpro/metrics/_aucc.py | New module containing AUCalibration extracted from the old _classes.py. |
| skpro/metrics/init.py | Updated public exports to import from the new per-metric modules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else: | ||
| y_true_np = y_true | ||
| if y_true_np.ndim == 1: | ||
| y_true_np = y_true.reshape(-1, 1) |
There was a problem hiding this comment.
y_true_np is the NumPy array, but the reshape is applied to y_true. If y_true is a pandas object (as allowed by the docstring), y_true.reshape(...) will raise. Reshape y_true_np instead (e.g., y_true_np = y_true_np.reshape(-1, 1)).
| y_true_np = y_true.reshape(-1, 1) | |
| y_true_np = y_true_np.reshape(-1, 1) |
| else: | ||
| y_true_np = y_true | ||
| if y_true_np.ndim == 1: | ||
| y_true_np = y_true.reshape(-1, 1) |
There was a problem hiding this comment.
y_true_np is the array you want to reshape, but the code reshapes y_true. This will fail for pandas inputs. Use y_true_np = y_true_np.reshape(-1, 1).
| y_true_np = y_true.reshape(-1, 1) | |
| y_true_np = y_true_np.reshape(-1, 1) |
| y_true_np = y_true | ||
|
|
||
| if y_true_np.ndim == 1: | ||
| y_true_np = y_true.reshape(-1, 1) |
There was a problem hiding this comment.
Same reshape issue as other interval metrics: y_true may be a pandas object and not support .reshape. Reshape y_true_np instead.
| y_true_np = y_true.reshape(-1, 1) | |
| y_true_np = y_true_np.reshape(-1, 1) |
| from skpro.metrics._crps import CRPS | ||
| from skpro.metrics._empirical_coverage import EmpiricalCoverage | ||
| from skpro.metrics._interval_width import IntervalWidth | ||
| from skpro.metrics._logloss_linearlized import LinearizedLogLoss |
There was a problem hiding this comment.
The module name _logloss_linearlized appears to be misspelled (should be _logloss_linearized). This makes the codebase harder to navigate and is easy to propagate into downstream imports. Consider renaming the file/module and updating this import accordingly.
| from skpro.metrics._logloss_linearlized import LinearizedLogLoss | |
| try: | |
| from skpro.metrics._logloss_linearized import LinearizedLogLoss | |
| except ImportError: | |
| from skpro.metrics._logloss_linearlized import LinearizedLogLoss |
|
|
||
|
|
||
| class LinearizedLogLoss(BaseDistrMetric): | ||
| r"""Lineararized logarithmic loss for distributional predictions. |
There was a problem hiding this comment.
Typo in the class docstring: 'Lineararized' should be 'Linearized'.
| r"""Lineararized logarithmic loss for distributional predictions. | |
| r"""Linearized logarithmic loss for distributional predictions. |
| return res | ||
|
|
||
| @classmethod | ||
| def get_test_params(self): |
There was a problem hiding this comment.
get_test_params is a @classmethod but uses self as the first parameter. While it technically works, it’s misleading and inconsistent with the other metrics in this PR. Rename the first parameter to cls and consider adding the optional parameter_set=\"default\" argument for interface consistency with other metric classes.
| def get_test_params(self): | |
| def get_test_params(cls, parameter_set="default"): |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…kpro into refactor-metric-classes
Refactors the
metricsmodule to contain private files with one metric class per private module each.The benefit is not having a gigantic file with all metric classes in it, and following the general pattern that is also used for estimators and other object types.
Also fixes some minor errors with regards to compatibility with 1D
numpyarrays.