Include more information in error about different number of data points

[jgabry]

Suggested by @bgoodri: when loo_compare() errors due to models have different numbers of data points, it would be helpful to the user if we print the number of data points instead of just saying they're different.

loo/R/loo_compare.R

Line 191 in 734c3be

stop("Not all models have the same number of data points.", call.=FALSE)

[jordandeklerk]

Hi @jgabry! We currently have an open issue related to this in arviz-stats here arviz-devs/arviz-stats#179. Wanted to see if we could possibly align on the best approach for this so we could display similar error messages for compare() in ArviZ.

I have the following modified errors messages right now. Wonder what you think of these:

For a non-valid method

if method.lower() not in available_methods:
    raise ValueError(
        f"Invalid method '{method}'. "
        f"Available methods: {', '.join(available_methods)}. "
        f"Use 'stacking' for robust model averaging as recommended in the original paper "
        f"https://arxiv.org/abs/1704.02030."
    )

For models with different number of observations

raise ValueError(
    f"Model '{val}' has {model_count} observations but expected {expected_count}. "
    f"All models must have the same number of observations. "
    f"First model has {expected_count} observations."
)

For loo results without pointwise values

raise ValueError(
    f"Model '{name}' is missing pointwise ELPD values. "
    f"Recalculate with loo(model, pointwise=True)."
)

For ELPD values computed via different methods

raise ValueError(
    f"All ELPD values must be computed using the same method. "
    f"Found mixed methods: '{first_kind}' and '{elpd_data.kind}'. "
    f"Please recalculate all models using the same method (e.g., all using 'loo')."
)

[jgabry]

Hey Jordan, good idea! And cool that you're working on Arviz (is that recent or am I forgetting that you were doing that for a while?).

raise ValueError(
f"Model '{val}' has {model_count} observations but expected {expected_count}. "
f"All models must have the same number of observations. "
f"First model has {expected_count} observations."
)

What would this look like when comparing more than two models?

raise ValueError(
f"All ELPD values must be computed using the same method. "
f"Found mixed methods: '{first_kind}' and '{elpd_data.kind}'. "
f"Please recalculate all models using the same method (e.g., all using 'loo')."
)

If I recall correctly, I think @avehtari is actually in favor of allowing users to compare K-fold and PSIS-LOO, with the idea being that even though it's not ideal it can still be useful. @avehtari is that correct?

[avehtari]

K-fold-CV with pointwise log scores can be very close to LOO, and thus it is useful to allow that comparison, but fine to provide informational message. loo_compare() function currently provides message

Comparing LOO-CV to K-fold-CV. ",
            "For a more accurate comparison use the same number of folds ",
            "or loo for all models compared.

Roaches case study illustrates comparison of loo and kfold results in a case where PSIS-LOO fails for one of the models, and for practical purposes this is fine

[jordandeklerk]

Hey Jordan, good idea! And cool that you're working on Arviz (is that recent or am I forgetting that you were doing that for a while?).

Thanks! I've been doing work in arviz-stats on PSIS-LOO-CV functionality for a couple of months now since March of this year, so fairly recent.

What would this look like when comparing more than two models?

Great point. Something like this should work to look at multiple models:

def _ic_matrix(ics):
    """Store the previously computed pointwise predictive accuracy values (ics) in a 2D matrix."""
    cols, _ = ics.shape
    rows = len(ics["elpd_i"].iloc[0])
    ic_i_val = np.zeros((rows, cols))

    first_model = ics.index[0]
    mismatches = []
    for val in ics.index:
        ic_len = len(ics.loc[val]["elpd_i"])
        if ic_len != rows:
            mismatches.append((val, ic_len))
    
    if mismatches:
        mismatch_details = ", ".join([f"'{name}' ({count})" for name, count in mismatches])
        raise ValueError(
            f"Models have inconsistent observation counts. "
            f"Model '{first_model}' has {rows} observations, but these models differ: {mismatch_details}. "
            f"All models must have the same number of observations."
        )

    for idx, val in enumerate(ics.index):
        ic = ics.loc[val]["elpd_i"]
        ic_i_val[:, idx] = ic

    return rows, cols, ic_i_val

This should return something like:

ValueError: Models have inconsistent observation counts. Model 'model_b' has 1 observations, but these models differ: 'model_d' (2), 'model_a' (8), 'model_c' (8). All models must have the same number of observations.

If I recall correctly, I think @avehtari is actually in favor of allowing users to compare K-fold and PSIS-LOO, with the idea being that even though it's not ideal it can still be useful. @avehtari is that correct?

Perfect, then I think something like this could work, following similar verbiage as the current error:

...
    precomputed_elpds = {
        name: elpd_data
        for name, elpd_data in compare_dict.items()
        if isinstance(elpd_data, ELPDData)
    }
    if precomputed_elpds:
        for name, elpd_data in precomputed_elpds.items():
            if elpd_data.elpd_i is None:
                raise ValueError(
                    f"Model '{name}' is missing pointwise ELPD values. "
                    f"Recalculate with loo(model, pointwise=True)."
                )
        
        methods_used = {}
        for name, elpd_data in precomputed_elpds.items():
            kind = elpd_data.kind
            if kind not in methods_used:
                methods_used[kind] = []
            methods_used[kind].append(name)
        
        if len(methods_used) > 1:
            warnings.warn(
                "Comparing LOO-CV to K-fold-CV. "
                "For a more accurate comparison use the same number of folds "
                "or loo for all models compared.",
                UserWarning
            )

This would look something like:

UserWarning: Comparing LOO-CV to K-fold-CV. For a more accurate comparison use the same number of folds or loo for all models compared.

I guess technically this warning would not make sense if a user changed the kind attribute of the ELPDData object. But I'm not sure why that would happen. In any case, just to cover all bases, we could just check the specific kind attribute and only raise this warning for loo vs loo_kfold types if we really needed to.

[jgabry]

ValueError: Models have inconsistent observation counts. Model 'model_b' has 1 observations, but these models differ: 'model_d' (2), 'model_a' (8), 'model_c' (8). All models must have the same number of observations.

Is model_b here just the model they input first? What if instead of treating that one as the one the rest are compared to, we just list the models and their number of observations? So maybe something like this:

All models must have the same number of observations, but models have inconsistent observation counts: 'model_b' (1), 'model_d' (2), 'model_a' (8), 'model_c' (8)

[jordandeklerk]

Is model_b here just the model they input first? What if instead of treating that one as the one the rest are compared to, we just list the models and their number of observations?

Yeah, model_b would be the first model that is input from the user. But I think just listing the models and their number of observations makes perfect sense as well.