Skip to content

refactor: simplify best_fit selection loop with max() #155

Description

@MAfarrag

Problem

The p-value selection logic in best_fit uses a manual loop with a sentinel value:

best_name = None
best_pvalue = -1.0
for name, info in fit_results.items():
    pvalue = info[criterion][1]
    if pvalue > best_pvalue:
        best_pvalue = pvalue
        best_name = name
return best_name, fit_results[best_name]

This is 6 lines for what is essentially a max() call. The sentinel -1.0 is a code smell — it works because p-values are always >= 0, but it's not self-evident.

Proposed Solution

Replace with max() using a key function:

best_name = max(
    self._fit_results,
    key=lambda name: getattr(self._fit_results[name], criterion).p_value,
)
return best_name, self._fit_results[best_name]

This is:

  • 2 lines instead of 6
  • No sentinel valuemax() handles empty-check naturally
  • More Pythonic — standard pattern for "find the item with the highest X"

Note

This uses getattr on the FitResult dataclass (from #152) where criterion is "ks" or "chisquare", matching the attribute names on FitResult. If #152 is not implemented yet, the dict-based version would be:

best_name = max(
    fit_results,
    key=lambda name: fit_results[name][criterion][1],
)

Scope

  • Replace the loop in best_fit with max()
  • Verify existing tests still pass (no behavioral change)

Depends On

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestpythonPull requests that update python code

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions