Skip to content

Conversation

@bejaeger
Copy link
Contributor

@bejaeger bejaeger commented Nov 4, 2025

Issue

Previously, the test was prone to statistical fluctuations because we were fitting a very small dataset.
This creates a dataset specific to the test and uses a few more samples to make the test more robust.

Copilot AI review requested due to automatic review settings November 4, 2025 15:41
@bejaeger bejaeger requested a review from a team as a code owner November 4, 2025 15:41
@bejaeger bejaeger requested review from brendan-priorlabs and removed request for a team November 4, 2025 15:41
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to make a test more robust by replacing a 3-class classification dataset with an easier 2-class dataset, reducing the likelihood of failures due to statistical fluctuations. The change is well-implemented. My feedback includes a suggestion to strengthen the performance assertions in the test to better leverage the simpler dataset and improve the test's ability to detect regressions.

Copy link
Contributor

Copilot AI left a comment

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 modifies the test_predict_logits_and_consistency test to use inline dataset generation instead of the shared X_y fixture. The test now creates its own classification dataset with specific parameters (60 samples, 2 classes, 3 features) rather than using the module-level fixture that generates a different dataset (60 samples, 3 classes, 5 features).

Key Changes:

  • Removed the X_y fixture parameter from the test function signature
  • Added inline sklearn.datasets.make_classification() call with custom parameters (2 classes vs 3, 3 features vs 5)

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

Copy link
Contributor

@brendan-priorlabs brendan-priorlabs left a comment

Choose a reason for hiding this comment

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

Thanks, @bejaeger!

@bejaeger bejaeger merged commit 5fb12b9 into main Nov 4, 2025
10 checks passed
oscarkey pushed a commit that referenced this pull request Nov 12, 2025
* Record copied public PR 588

* More robust performance test (#588)

(cherry picked from commit 5fb12b9)

---------

Co-authored-by: mirror-bot <mirror-bot@users.noreply.github.com>
Co-authored-by: Benjamin Jaeger <benjamin@priorlabs.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants