Skip to content

[c++] Lightly refactor MapMetric, fix small bug, and add unit tests#7202

Open
pgrinspan wants to merge 10 commits intolightgbm-org:masterfrom
pgrinspan:master
Open

[c++] Lightly refactor MapMetric, fix small bug, and add unit tests#7202
pgrinspan wants to merge 10 commits intolightgbm-org:masterfrom
pgrinspan:master

Conversation

@pgrinspan
Copy link
Copy Markdown

@pgrinspan pgrinspan commented Mar 15, 2026

Contributes to #7021

  • Simplify CalMapAtK a tad
  • Ignore queries with no positive labels
  • Unify weighted/unweighted Eval paths via GetQueryWeight helper
  • Add test_map_metric.cpp with tests for CalMapAtK and Eval

Pierre Grinspan added 2 commits March 14, 2026 19:40
- Simplify CalMapAtK a tad
- Ignore queries with no positive labels
- Unify weighted/unweighted Eval paths via GetQueryWeight helper
- Add test_map_metric.cpp with tests for CalMapAtK and Eval
@jameslamb jameslamb changed the title Lightly refactor MapMetric and add unit tests [c++] Lightly refactor MapMetric and add unit tests Mar 15, 2026
Copy link
Copy Markdown
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks very much!

In the future, please do link to previous conversations when a PR is related to one (I've added a link to #7021 in the description). This helps us connect related conversations in the project.

@metpavel when you have time could you please review this?

@jameslamb jameslamb requested a review from metpavel March 15, 2026 04:28
@jameslamb
Copy link
Copy Markdown
Member

By the way @pgrinspan it looks like you local git configuration is not set up to tie your commits to your GitHub account. See #7143 (comment) for details on how to fix that.

@pgrinspan
Copy link
Copy Markdown
Author

By the way @pgrinspan it looks like you local git configuration is not set up to tie your commits to your GitHub account. See #7143 (comment) for details on how to fix that.

Ok, I've applied it.
I'm guessing it's not retroactive but hopefully not a big issue? Let me know if it is, and what to do about it - thanks!

@pgrinspan pgrinspan changed the title [c++] Lightly refactor MapMetric and add unit tests [c++] Lightly refactor MapMetric, fix small bug, and add unit tests Mar 15, 2026
@jameslamb
Copy link
Copy Markdown
Member

Thanks, recent commit looks good!

It doesn't apply retroactively. You could do that by force-pushing over those previous commits but don't bother....when this is merged we'll squash everything into a single commit and it'll be attributed to your GitHub account.

Just noting because:

  1. it might cause issues for you in other GitHub projects
  2. it might mean that your next PR will incorrectly be labeled as coming from a "new contributor" and again require us to manually run the automated tests

Thanks for taking the time to contribute, we greatly appreciate it 😊

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants