Skip to content

Learning to rank tuning and uncertainty - #45

Open
thomasATbayer wants to merge 79 commits into
mainfrom
learningToRankTuningAndUncertainty
Open

Learning to rank tuning and uncertainty#45
thomasATbayer wants to merge 79 commits into
mainfrom
learningToRankTuningAndUncertainty

Conversation

@thomasATbayer

@thomasATbayer thomasATbayer commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Add CatBoost Ranking, Uncertainty, and Stability Analysis

Overview

This pull request adds a complete CatBoost ranking workflow to MotherML, together with uncertainty estimation, rank-aware analysis utilities, hyperparameter tuning support, and compatibility safeguards for cross-validation and model serialization.

The main functionality being added to main is the ability to train CatBoost rankers through the Mother framework, obtain either ranking scores or within-group ranks, quantify uncertainty across virtual ensembles, and analyse ranking stability at group and top-k level.

What This PR Adds

CatBoost Ranking Model

CatboostRankerMother provides a Mother-compatible CatBoost ranker with:

  • A consistent estimator interface compatible with the existing Mother pipelines and tuners.
  • Ranking-specific model validation through model_type="ranking".
  • Support for CatBoost ranking losses, including YetiRank, PairLogit, QueryRMSE, QuerySoftMax, and pairwise variants where the CatBoost constraints are satisfied.
  • Support for ranking-specific parameters such as top for YetiRank and max_pairs for PairLogit losses.
  • Metadata-routing support for passing group_id during fitting and scoring.
  • Scikit-learn-compatible get_params(), set_params(), cloning, and serialization behavior.

Score and Rank Predictions

The ranker can return either raw CatBoost scores or 1-based ranks:

  • The default prediction is the raw ranking score from CatBoost.
  • use_ranks=True converts scores into 1-based ranks within the supplied ranking group.
  • Rank 1 is assigned to the highest-scoring item.
  • Groupwise prediction helpers preserve the original row order and calculate ranks independently for each group.
  • Optional normalization by group size is available for rank-based workflows.

This makes the same model useful both for downstream score-based ranking metrics and for users who need an explicit rank position for every item.

Ranking Uncertainty from Virtual Ensembles

CatboostRankerMother.predict_uncertainty() uses CatBoost virtual ensembles through virtual_ensembles_predict, via the shared mother.ml.utils.get_virtual_prediction() helper.

The implementation does not use staged_predict snapshots and does not calculate IQR-based uncertainty by default. Its uncertainty semantics are:

  • pred contains raw scores by default.
  • With use_ranks=True, pred contains 1-based ranks.
  • mean_predictions contains the mean virtual-ensemble score or rank.
  • knowledge_uncertainty is the standard deviation across virtual-ensemble scores or ranks.
  • data_uncertainty is None for ranking.
  • total_uncertainty is None for ranking because this implementation reports epistemic uncertainty only.

The method supports uncertainty_for_opt=True to return only the uncertainty column needed by optimization workflows.

Raw Ensemble Scores and Quantile Analysis

For standalone ranking analysis, the ranker supports:

  • return_raw=True, returning (uncertainty_df, raw_scores), where raw_scores contains one raw score per sample and virtual ensemble.
  • return_quantiles=True, adding empirical score or rank quantiles such as score_q25, score_q50, and score_q75.
  • Raw score access for custom per-ensemble rank calculations, score variance, and ranking stability analysis.

The raw score matrix enables analysis beyond the aggregate uncertainty DataFrame, including how consistently items appear near the top of a ranking across virtual ensembles.

Groupwise and Top-k Stability Analysis

New ranking utilities support stability analysis at the level users normally inspect rankings:

  • ranker_predict_for_groups() predicts scores or ranks independently within each group.
  • ranker_predict_uncertainty_for_groups() runs uncertainty estimation independently for each group and restores the original input order.
  • topk_score_variance() identifies top-k items and computes their score variance across virtual ensembles.
  • groupwise_topk_analysis() calculates top-k membership probability, top-k score variance, and consensus top-k membership for each group.
  • Ranking helpers convert score arrays and score matrices into stable, 1-based rank arrays while preserving input order.

These utilities make it possible to distinguish stable rankings from ambiguous rankings and to identify items whose top-k membership changes across virtual ensemble members.

The groupwise uncertainty helper intentionally rejects return_raw=True, because raw results are tuples and require explicit per-group aggregation. Users who need raw scores should call the ranker directly for each group.

Ranking Hyperparameter Tuning

The ranker integrates with MotherTuner and exposes ranking-specific search behavior:

  • Tree structure and boosting type can be tuned through the shared CatBoost hyperparameter machinery.
  • Loss-function tuning can select compatible ranking losses and their associated parameters.
  • Pairwise losses are only considered when the selected tree structure and boosting configuration are compatible with CatBoost requirements.
  • Incompatible pairwise configurations are rejected or disabled clearly rather than producing invalid trials.
  • User-defined max_pairs is applied when the effective selected loss supports it, especially PairLogit.
  • Unsupported losses such as YetiRank and QueryRMSE are left unchanged when max_pairs is configured.
  • Updating top through set_params() updates the effective YetiRank mode and top suffix when no explicit loss is supplied in the same call.
  • Explicitly supplied or Optuna-selected loss functions remain authoritative.
  • Loss suffix formatting is consistent with construction, using : for a bare loss and ; for additional parameters.

This allows the ranker to be tuned without losing ranking-specific constraints or silently applying parameters to losses that do not support them.

Gaussian-Process Compatibility

The CatBoost Gaussian-process regressor remains supported and its behavior is made explicit:

  • Gaussian-process posterior sampling does not expose boosting, tree-structure, or loss-function tuning options through its public constructor.
  • Those tuning modes are disabled internally and remain False.
  • Legacy tuning fields are consumed and discarded when loading older serialized states.
  • GP cloning, pickling, state restoration, fitting, prediction, uncertainty estimation, and optimization remain covered by tests.

Cross-validation and Pipeline Integration

The ranking model works with the Mother pipeline and cross-validation utilities, including group-aware workflows and rank-aware scoring.

mother_cv() forwards uncertainty keyword arguments but requires one uncertainty DataFrame per fold. It now rejects tuple-valued uncertainty results before generic DataFrame conversion. For the CatBoost ranker, one example is return_raw=True, which returns (uncertainty_df, raw_scores) and is intended for standalone analysis rather than cross-validation aggregation.

Dependency and Code-Quality Improvements

Shared ranking helpers now live in mother.ml.utils instead of importing them from mother.ml.models.m_catboost. This removes the circular dependency between the utility module and the CatBoost model module while preserving compatibility aliases for existing m_catboost users.

CatBoost ranking integration tests that train real models and run virtual-ensemble uncertainty are marked as slow so they do not unnecessarily delay the default unit-test suite.

Validation

The implementation is covered by focused tests for:

  • Rank score and 1-based rank conversion.
  • Groupwise score and rank prediction.
  • Virtual-ensemble uncertainty and output schema.
  • Raw score and quantile output modes.
  • Top-k membership probability and score variance.
  • Groupwise uncertainty aggregation.
  • Pairwise-loss compatibility and conditional max_pairs behavior.
  • mother_cv forwarding and tuple-output rejection.
  • Gaussian-process initialization, fitting, uncertainty, cloning, pickling, and state persistence.
  • CatBoost ranking pipeline construction and tuning integration.

The slow-test suite also identified stale GP assertions expecting the old public tuning flags; those assertions were updated to verify that the flags are absent from the public parameter API while remaining disabled internally.

User-Facing Contract

The ranking uncertainty contract added by this PR is therefore:

virtual_ensembles_predict -> score/rank dispersion -> knowledge_uncertainty

It is not a staged-predict/IQR implementation. Reviewers and downstream users should interpret knowledge_uncertainty as virtual-ensemble standard deviation, with total_uncertainty=None for the ranker.

Copilot AI lite review requested due to automatic review settings July 1, 2026 09:32
@thomasATbayer thomasATbayer linked an issue Jul 1, 2026 that may be closed by this pull request
@thomasATbayer thomasATbayer linked an issue Jul 1, 2026 that may be closed by this pull request
@thomasATbayer thomasATbayer added the enhancement New feature or request label Jul 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the CatboostRankerMother wrapper to support rank-based predictions and staged-prediction uncertainty estimates, and adds more flexible hyperparameter tuning controls in the shared CatBoost tuning base class.

Changes:

  • Added tune_loss_function to _CatboostHyperParams to optionally keep the constructor loss fixed during Optuna tuning.
  • Introduced rank utilities and rank-normalization options (scores_to_ranks, predict(..., ranks=..., normalize_by_group_size=...)) plus a rewritten predict_uncertainty for rank uncertainty via staged_predict.
  • Expanded CatboostRankerMother tuning options (pairwise loss inclusion, top, max_pairs) and implemented estimator parameter/pickling helpers.

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated 4 comments.

File Description
uv.lock Bumps the mother-ml package version to 1.0.1.
src/mother/ml/models/m_catboost.py Adds rank conversion helper, new tuning flags, rank prediction/normalization, and staged rank-uncertainty estimation for CatboostRankerMother.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/mother/ml/models/m_catboost.py Outdated
Comment thread src/mother/ml/models/m_catboost.py Outdated
Comment thread src/mother/ml/models/m_catboost.py Outdated
Comment thread src/mother/ml/models/m_catboost.py Outdated
Copilot AI review requested due to automatic review settings July 1, 2026 09:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 2 changed files in this pull request and generated 6 comments.

Comment thread src/mother/ml/models/m_catboost.py Outdated
Comment thread src/mother/ml/models/m_catboost.py Outdated
Comment thread src/mother/ml/models/m_catboost.py
Comment thread src/mother/ml/models/m_catboost.py
Comment thread src/mother/ml/models/m_catboost.py
Comment thread src/mother/ml/models/m_catboost.py
Copilot AI review requested due to automatic review settings July 1, 2026 10:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 2 changed files in this pull request and generated 10 comments.

Comment thread src/mother/ml/models/m_catboost.py Outdated
Comment thread src/mother/ml/models/m_catboost.py
Comment thread src/mother/ml/models/m_catboost.py Outdated
Comment thread src/mother/ml/models/m_catboost.py
Comment thread src/mother/ml/models/m_catboost.py
Comment thread src/mother/ml/models/m_catboost.py
Comment thread src/mother/ml/models/m_catboost.py
Comment thread src/mother/ml/models/m_catboost.py
Comment thread src/mother/ml/models/m_catboost.py
Comment thread src/mother/ml/models/m_catboost.py Outdated
Copilot AI review requested due to automatic review settings July 1, 2026 10:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 2 changed files in this pull request and generated 3 comments.

Comment thread src/mother/ml/models/m_catboost.py
Comment thread src/mother/ml/models/m_catboost.py
Comment thread src/mother/ml/models/m_catboost.py Outdated
Copilot AI review requested due to automatic review settings July 1, 2026 11:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/mother/ml/models/m_catboost.py:77

  • ensure_metadata_routing() is defined but not referenced anywhere in the codebase. Keeping an unused decorator (especially one that mutates global sklearn config) increases maintenance cost and can confuse readers about whether metadata routing is auto-enabled for rankers.
def ensure_metadata_routing(func: Callable) -> Callable:
    """
    Decorator to ensure metadata routing is enabled before executing a function.

    This decorator checks if sklearn's metadata routing is enabled and activates it

src/mother/ml/models/m_catboost.py:2207

  • In suggested_params_loss(), max_pairs is appended to PairLogit losses whenever max_pairs is non-None and > 0, but this allows non-integer values (e.g., True, 2.5) to produce an invalid CatBoost loss string like PairLogit:max_pairs=True. This contradicts the stricter validation used elsewhere in the ranker and can cause Optuna trials to fail unexpectedly.
                loss_function += f":max_pairs={self.max_pairs}"

            suggested_params[prefix + "loss_function"] = loss_function

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/mother/ml/utils.py:750

  • topk_score_variance() documents reference_ranks as 1-based ranks but doesn’t validate the range. If a caller passes 0-based ranks or values outside [1, n_samples], items can be incorrectly treated as top-k without any error. Adding a simple range check would make the API safer and prevent silent misanalysis.
    reference_ranks = np.asarray(reference_ranks).reshape(-1)
    if len(reference_ranks) != arr.shape[0]:
        raise ValueError("reference_ranks must have one entry per score_ensembles row.")
    if not np.isfinite(reference_ranks).all():
        raise ValueError("reference_ranks must contain only finite values.")

src/mother/ml/utils.py:702

  • topk_rank_disagreement() assumes rank_ensembles contains 1-based ranks, but it only validates finiteness. If callers accidentally pass 0-based ranks (or any values outside [1, n_items]), the function will silently over-count top-k membership (e.g., rank=0 always counts as top-k). Consider validating the rank range explicitly to fail fast on invalid inputs.

This issue also appears on line 746 of the same file.

    if not np.isfinite(arr).all():
        raise ValueError("rank_ensembles must contain only finite values.")
    in_topk = arr <= k
    return in_topk.mean(axis=1)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/mother/ml/utils.py:491

  • virtual_ensembles_count validation currently requires isinstance(..., int), which rejects NumPy integer types (e.g. np.int64(10)) even though they are valid positive integers per the error message. This can cause unexpected ValueErrors for callers using NumPy-derived counts; consider accepting np.integer and normalizing to a built-in int after validation.
    if (
        not isinstance(virtual_ensembles_count, int)
        or isinstance(virtual_ensembles_count, bool)
        or virtual_ensembles_count < 1
    ):
        raise ValueError(f"virtual_ensembles_count must be a positive integer, got {virtual_ensembles_count}.")

    module_logger.info("Using catboost's builtin uncertainty prediction")

src/mother/ml/utils.py:702

  • topk_rank_disagreement documents that rank_ensembles contains 1-based ranks, but it never validates that the values are within [1, n_items]. Out-of-range (or 0-based) ranks will silently skew the computed top-k membership probabilities; adding an explicit range check makes the contract enforceable and failures easier to diagnose.
    arr = np.asarray(rank_ensembles)
    if arr.ndim != 2:
        raise ValueError(f"Expected 2D rank_ensembles, got {arr.ndim}D.")
    if k < 1:
        raise ValueError(f"k must be >= 1, got {k}.")
    if k > arr.shape[0]:
        raise ValueError(f"k must be <= the number of items ({arr.shape[0]}), got {k}.")
    if not np.isfinite(arr).all():
        raise ValueError("rank_ensembles must contain only finite values.")
    in_topk = arr <= k
    return in_topk.mean(axis=1)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/mother/ml/models/m_catboost.py:103

  • ensure_metadata_routing is defined in this module but does not appear to be used anywhere in src/ (only the definition exists). This also pulls in wraps, skl_get_config, and skl_set_config solely for that unused decorator, which increases maintenance surface and can trigger unused-import lint failures. Consider either removing this decorator (and its related imports) or actually applying it where intended (and reconciling with the class docs that say metadata routing is not enabled automatically).
def ensure_metadata_routing(func: Callable) -> Callable:
    """
    Decorator to ensure metadata routing is enabled before executing a function.

    This decorator checks if sklearn's metadata routing is enabled and activates it
    if necessary. It's particularly useful for initializing ranking models that require
    metadata routing for passing additional parameters like group_id.

    Parameters
    ----------
    func : Callable
        The function to be decorated (typically __init__ of a ranking model)

    Returns
    -------
    Callable
        The wrapped function with metadata routing ensured
    """

    @wraps(func)
    def wrapper(*args, **kwargs):
        use_metadata_routing: bool = bool(skl_get_config().get("enable_metadata_routing", False))
        if not use_metadata_routing:
            module_logger.warning(
                "Metadata routing is not enabled, enabling it now. This may cause issues in passing "
                "training arguments to other sklearn objects."
            )
            skl_set_config(enable_metadata_routing=True)  # NOSONAR
        return func(*args, **kwargs)

    return wrapper

src/mother/ml/utils.py:783

  • groupwise_topk_analysis's docstring says uncertainty_df must contain mean_predictions and knowledge_uncertainty, but the implementation never reads those columns (it only copies the frame and appends new columns). This is misleading for callers and makes the contract stricter than the code actually requires.
    uncertainty_df : pd.DataFrame
        Output from ``predict_uncertainty`` containing ``mean_predictions`` and
        ``knowledge_uncertainty`` columns.
    score_ensembles : np.ndarray, shape (n_samples, n_ensembles)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

src/mother/ml/models/m_catboost.py:77

  • ensure_metadata_routing is defined but not used anywhere in the repo, and it pulls in extra imports (wraps, skl_get_config, skl_set_config) plus a global side effect (mutating sklearn config). If metadata routing should not be auto-enabled (per the ranker docs), consider removing this unused decorator and its imports to avoid confusion.
def ensure_metadata_routing(func: Callable) -> Callable:
    """
    Decorator to ensure metadata routing is enabled before executing a function.

    This decorator checks if sklearn's metadata routing is enabled and activates it

Comment thread src/mother/ml/utils.py
Comment on lines 433 to +442
def get_virtual_prediction(
X: pd.DataFrame,
model: typing.Union[
CatBoostRegressor,
CatBoostClassifier,
CatBoostRanker,
],
virtual_ensembles_count: int = 10,
thread_count: int = 1,
) -> pd.DataFrame:
) -> typing.Union[pd.DataFrame, typing.Tuple[pd.DataFrame, np.ndarray]]:
Comment thread src/mother/ml/models/m_catboost.py Outdated
Comment on lines +2347 to +2354
group_arr = _validate_ranking_group_id(group_id, len(X))
frames: list[pd.DataFrame] = []

for idx in _iter_ranking_group_indices(group_arr):
X_group = X.iloc[idx] if isinstance(X, pd.DataFrame) else X[idx]
frames.append(model.predict_uncertainty(X_group, **kwargs))

return pd.concat(frames).loc[X.index]

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/mother/ml/models/m_catboost.py:2139

  • CatboostRankerMother.suggested_params_loss() decides whether Pairwise losses are allowed using boosting_type = suggested_params.get(..., "Plain"). However, when grow_policy == "SymmetricTree" and tune_boosting_type=False, _CatboostHyperParams.get_hyperparameter_space() does not populate boosting_type in suggested_params, so this defaults to "Plain" even if the model was constructed with (fixed) boosting_type="Ordered". That can make Optuna suggest *Pairwise losses that are incompatible with the actual fixed boosting type.

To avoid invalid trials, derive the fallback boosting_type from the model’s current parameters when it isn’t present in suggested_params.

        grow_policy: Optional[str] = suggested_params.get(prefix + "grow_policy")
        boosting_type: str = suggested_params.get(prefix + "boosting_type", "Plain")
        can_use_pairwise: bool = grow_policy == "SymmetricTree" and boosting_type == "Plain"

Comment on lines +785 to +791
intermediate_performance_data: pd.DataFrame = val_estimator.predict_uncertainty(X.iloc[test_idx, :], **kwargs)

if isinstance(intermediate_performance_data, tuple):
raise TypeError(
"mother_cv requires predict_uncertainty to return a pandas DataFrame; "
"tuple-valued predict_uncertainty outputs are not supported."
)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

Comment thread src/mother/ml/utils.py
Comment on lines +792 to +796
For each ranking group, computes:
- ``topk_prob``: fraction of ensembles disagreeing about each item's top-k membership
- ``topk_score_var``: score variance for items in the top-k (0 otherwise)
- ``topk_member``: whether the item is in the consensus top-k (based on mean rank)

Comment on lines 783 to +787
module_logger.debug("The target values are being predicted")

intermediate_performance_data: pd.DataFrame = val_estimator.predict_uncertainty(X.iloc[test_idx, :])
intermediate_performance_data: pd.DataFrame = val_estimator.predict_uncertainty(X.iloc[test_idx, :], **kwargs)

if isinstance(intermediate_performance_data, tuple):

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/mother/ml/models/m_catboost.py:77

  • ensure_metadata_routing is defined here but is not used anywhere in the repo. Keeping an unused decorator that mutates global sklearn configuration increases maintenance burden and may confuse readers about whether metadata routing is automatically enabled for rankers.
def ensure_metadata_routing(func: Callable) -> Callable:
    """
    Decorator to ensure metadata routing is enabled before executing a function.

    This decorator checks if sklearn's metadata routing is enabled and activates it

Comment on lines 671 to 685
def mother_cv(
estimator: Union[ml.PipelineWithHyperparameterRooting, ml.AbstractMotherPipeline],
*,
cv: skl_model_sel.BaseCrossValidator,
inner_cv: Optional[skl_model_sel.BaseCrossValidator] = None,
X: pd.DataFrame,
y: Union[pd.Series, pd.DataFrame],
groups: Optional[pd.DataFrame] = None,
tuner: Optional[MotherTuner] = None,
hyperparameter_space_function: Optional[Callable] = None,
default_parameters: Optional[dict] = None,
prediction_prefix: str = "pred_",
return_estimators: bool = False,
**kwargs: Any,
) -> Union[pd.DataFrame, tuple[pd.DataFrame, dict[str, Any]]]:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/mother/ml/utils.py:512

  • get_virtual_prediction() calls model.virtual_ensembles_predict(...) in the non-ranker branch without first validating that model is actually a CatBoost model instance. If a caller passes None or an unexpected object, this will raise an AttributeError instead of the intended ValueError (and the final else: raise ValueError(...) becomes effectively unreachable for such inputs). Add an early isinstance guard so invalid inputs fail with a clear, consistent exception.
    module_logger.info("Using catboost's builtin uncertainty prediction")

    if isinstance(model, CatBoostRanker):

src/mother/ml/models/m_catboost.py:77

  • ensure_metadata_routing() is defined in this module but does not appear to be used anywhere (no decorators/call sites). Keeping an unused helper that mutates global sklearn config (set_config(enable_metadata_routing=True)) is confusing and risks being applied later without noticing the side effects; either apply it intentionally (with clear rationale) or remove it to avoid dead code.
def ensure_metadata_routing(func: Callable) -> Callable:
    """
    Decorator to ensure metadata routing is enabled before executing a function.

    This decorator checks if sklearn's metadata routing is enabled and activates it

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/mother/ml/models/m_catboost.py:80

  • ensure_metadata_routing() is defined here but is not referenced anywhere in src/mother/ml/models/m_catboost.py (and there is also a separate ensure_metadata_routing decorator in test/unit/conftest.py). Leaving an unused public-looking helper in the production module increases maintenance burden and can confuse readers about whether metadata routing is auto-enabled. Consider removing it from this module (or applying it where intended) and keeping a single authoritative implementation.
def ensure_metadata_routing(func: Callable) -> Callable:
    """
    Decorator to ensure metadata routing is enabled before executing a function.

    This decorator checks if sklearn's metadata routing is enabled and activates it
    if necessary. It's particularly useful for initializing ranking models that require
    metadata routing for passing additional parameters like group_id.

src/mother/ml/utils.py:703

  • topk_rank_disagreement() currently returns 1 - P(in_topk) (i.e., the probability an item is not in the top-k). That contradicts the docstring (“0.0 means every ensemble agrees about the item's top-k membership”) and the PR description’s “top-k membership probability”: if an item is never in the top-k across ensembles, all ensembles agree it is not in the top-k, but this function returns 1.0 (max disagreement). Please clarify the intended metric and either (a) change the implementation to a true disagreement measure (0 when always-in or always-out), or (b) rename/update the docstring/column names/tests to reflect that this is actually an exclusion probability.
    For each sample, returns one minus the fraction of ensemble members that
    place it in the top-k positions. A value of 0.0 means every ensemble agrees
    about the item's top-k membership; values near 1 indicate disagreement.

    Parameters
    ----------

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/mother/pipeline_utils.py:785

  • predict_uncertainty() is now called with **kwargs and explicitly allowed to return a tuple (which is handled immediately after), but the variable is still annotated as pd.DataFrame. With strict typing (mypy), this is inconsistent and can fail type-checking. Annotate as Any (or remove the annotation) since the code intentionally supports non-DataFrame intermediate values before normalization/validation.
        intermediate_performance_data: pd.DataFrame = val_estimator.predict_uncertainty(X.iloc[test_idx, :], **kwargs)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.

Comment on lines 680 to +684
hyperparameter_space_function: Optional[Callable] = None,
default_parameters: Optional[dict] = None,
prediction_prefix: str = "pred_",
return_estimators: bool = False,
**kwargs: Any,

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

src/mother/pipeline_utils.py:791

  • The TypeError message is inaccurate: mother_cv can accept non-DataFrame outputs (it converts them to a DataFrame below), it just cannot accept tuple outputs. This wording may mislead estimator authors debugging their predict_uncertainty implementations.
        intermediate_performance_data: pd.DataFrame = val_estimator.predict_uncertainty(X.iloc[test_idx, :], **kwargs)

        if isinstance(intermediate_performance_data, tuple):
            raise TypeError(
                "mother_cv requires predict_uncertainty to return a pandas DataFrame; "
                "tuple-valued predict_uncertainty outputs are not supported."
            )

src/mother/ml/models/m_catboost.py:429

  • __setstate__ uses state.pop("tune_loss_function") without a default. Unpickling older CatboostRegressorMother objects (saved before tune_loss_function existed) will raise KeyError, which contradicts the PR description’s “serialization compatibility/legacy fields” safeguards.
    def __setstate__(self, state):
        self.target_type = state.pop("target_type", "single_target")
        self.tune_boosting_type = state.pop("tune_boosting_type", False)
        self.tune_loss_function = state.pop("tune_loss_function")
        self.model_type = state.pop("model_type", "regression")

src/mother/ml/models/m_catboost.py:1422

  • __setstate__ uses state.pop("tune_loss_function") without a default. Unpickling older CatboostClassifierMother objects (saved before tune_loss_function existed) will raise KeyError, which contradicts the PR description’s “serialization compatibility/legacy fields” safeguards.
    def __setstate__(self, state):
        self.target_type = state.pop("target_type", "single_target")
        self.tune_boosting_type = state.pop("tune_boosting_type", False)
        self.tune_loss_function = state.pop("tune_loss_function")
        self.model_type = state.pop("model_type", "classification_binary")
        self.tune_tree_structure_type = state.pop("tune_tree_structure_type", True)

src/mother/ml/models/m_catboost.py:77

  • The ensure_metadata_routing decorator is defined but not referenced anywhere in this module. Keeping unused code here makes the ranking implementation harder to follow and suggests a behavior (auto-enabling global sklearn metadata routing) that does not actually occur.
def ensure_metadata_routing(func: Callable) -> Callable:
    """
    Decorator to ensure metadata routing is enabled before executing a function.

    This decorator checks if sklearn's metadata routing is enabled and activates it

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/mother/ml/models/m_catboost.py:77

  • ensure_metadata_routing is introduced as a helper that mutates global sklearn config, but it is not referenced anywhere in this module. Keeping an unused decorator (especially one with global side effects) is confusing and increases maintenance burden; either apply it where needed or remove it and the associated imports to avoid implying metadata routing is auto-enabled.
def ensure_metadata_routing(func: Callable) -> Callable:
    """
    Decorator to ensure metadata routing is enabled before executing a function.

    This decorator checks if sklearn's metadata routing is enabled and activates it

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/mother/pipeline_utils.py:785

  • This assignment is type-annotated as a pd.DataFrame, but the code immediately handles tuple-valued outputs (and non-DataFrame outputs) at runtime. Keeping the explicit pd.DataFrame annotation here is misleading and can confuse static analysis (e.g., making the tuple-guard look unreachable).
        intermediate_performance_data: pd.DataFrame = val_estimator.predict_uncertainty(X.iloc[test_idx, :], **kwargs)

src/mother/ml/utils.py:799

  • The docstring says topk_disagreement_prob is a “fraction of ensembles disagreeing”, but topk_rank_disagreement() returns 2 * p * (1 - p) (pairwise disagreement probability between two randomly chosen ensemble members), which is a different quantity. This is misleading for users interpreting the output.
    - ``topk_disagreement_prob``: fraction of ensembles disagreeing about each item's top-k membership

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable kwargs for mother_cv Enable Ranking Loss Function Tuning Introduce an option to turn of loss function tuning

3 participants