Skip to content

[BUG] Fix Benchmarking duplicate estimator label collisions#299

Open
ashum9 wants to merge 4 commits into
gc-os-ai:mainfrom
ashum9:codex/benchmarking-estimator-labels
Open

[BUG] Fix Benchmarking duplicate estimator label collisions#299
ashum9 wants to merge 4 commits into
gc-os-ai:mainfrom
ashum9:codex/benchmarking-estimator-labels

Conversation

@ashum9

@ashum9 ashum9 commented Apr 5, 2026

Copy link
Copy Markdown

Closes #297.

What this changes

  • disambiguates duplicate estimator class names in Benchmarking results with deterministic 1-based suffixes
  • preserves the original class name when there is only one estimator of that class
  • adds a regression test proving two DummyClassifier instances no longer overwrite one another
  • adds a regression test proving distinct estimator classes keep their original labels

Validation

  • pytest -q pyaptamer/benchmarking/tests/test_benchmarking_core.py

Copilot AI review requested due to automatic review settings April 5, 2026 20:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes a Benchmarking result-collision bug where multiple estimators of the same class overwrote each other by generating deterministic, disambiguated estimator display names (with 1-based suffixes only when needed), and adds regression tests for both duplicate and unique estimator naming.

Changes:

  • Add _get_estimator_names() to generate stable estimator labels and suffix duplicates (e.g., DummyClassifier[1], DummyClassifier[2]).
  • Update Benchmarking.run() to key results by these stable labels instead of raw class names.
  • Add regression tests to ensure duplicate estimator classes stay distinct and unique classes keep original labels.

Reviewed changes

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

File Description
pyaptamer/benchmarking/_base.py Introduces stable estimator naming and uses it to prevent result-row overwrites for duplicate estimator classes.
pyaptamer/benchmarking/tests/test_benchmarking_core.py Adds regression coverage for duplicate-name disambiguation and unique-name preservation.

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

Comment thread pyaptamer/benchmarking/_base.py Outdated
Comment thread pyaptamer/benchmarking/_base.py Outdated

@siddharth7113 siddharth7113 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

left the comment to change the suffix structure, otherwise looks good.


summary = bench.run()

assert ("DummyClassifier[1]", "accuracy_score") in summary.index

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this notation wouldn't be right, and could be confusing in downstream usage, user would have to make a call something like this df.loc["DummyClassifier[1]"] , I would suggest to index along with strategy something to make it either like DummyClassifer_1 or based on Pattern should be fine as well.

@ashum9

ashum9 commented Apr 10, 2026

Copy link
Copy Markdown
Author

@siddharth7113 Thanks for the feedback. I updated the duplicate-estimator label format to use underscore suffixes instead of brackets.

Now duplicate estimators of the same class are labeled deterministically as DummyClassifier_1, DummyClassifier_2 (1-based, only when needed), while unique estimator classes keep their original class name.

Updated the regression tests accordingly; pytest -q pyaptamer/benchmarking/tests/test_benchmarking_core.py passes.

@siddharth7113 siddharth7113 marked this pull request as ready for review April 12, 2026 18:28
@siddharth7113

Copy link
Copy Markdown
Collaborator

Please follow PR guidelines and make sure to install and run pre-commit , the current CI checks are failing.

@siddharth7113 siddharth7113 requested a review from satvshr April 20, 2026 13:00
@siddharth7113

Copy link
Copy Markdown
Collaborator

cc @satvshr , for your feedback

assert len(summary) == 2


def test_benchmarking_preserves_unique_estimator_names():

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Was this also a problem which you solved or are you adding a random test

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.

[BUG] Benchmarking collapses multiple estimators of the same class into one result row

4 participants