Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes and changes to the ranking metrics computed on cpu #5380

Merged
merged 4 commits into from Mar 3, 2020

Conversation

sriramch
Copy link
Contributor

@sriramch sriramch commented Mar 2, 2020

  • auc/aucpr ranking metric accelerated on cpu
  • fixes to the auc/aucpr metrics

it may help if you view the changes by ignoring the whitespace changes.

next, i'll create a pr for a gpu metric registry to decouple the cpu and gpu implementations

@RAMitchell @trivialfis @rongou - please review

  - auc/aucpr ranking metric accelerated on cpu
  - fixes to the auc/aucpr metrics

next, i'll create a pr for a gpu metric registry to decouple the cpu and gpu implementations
src/metric/rank_metric.cc Show resolved Hide resolved
src/metric/rank_metric.cc Show resolved Hide resolved
@RAMitchell
Copy link
Member

What are the fixes specifically?

}

// we need pos > 0 && neg > 0
if (total_pos <= 0.0 || total_neg <= 0.0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the fix.

Copy link
Member

Choose a reason for hiding this comment

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

So the fix is trigger a fatal error when only one label type exists locally? e.g. in a distributed partition.

Copy link
Member

Choose a reason for hiding this comment

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

Nvm looks like it just ignores the partition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if labels aren't diverse across all groups even in a non distributed environment.

src/metric/rank_metric.cc Show resolved Hide resolved
src/metric/rank_metric.cc Show resolved Hide resolved
@mli
Copy link
Member

mli commented Mar 2, 2020

Codecov Report

Merging #5380 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5380   +/-   ##
=======================================
  Coverage   84.07%   84.07%           
=======================================
  Files          11       11           
  Lines        2411     2411           
=======================================
  Hits         2027     2027           
  Misses        384      384           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b97eaf...e4509c4. Read the comment docs.

const auto &h_labels = info.labels_.ConstHostVector();
const auto &h_preds = preds.ConstHostVector();

#pragma omp parallel reduction(+:sum_auc, auc_error) if (ngroups > 1)
Copy link
Member

Choose a reason for hiding this comment

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

I spent some time working on the deterministic algorithm recently: #5361 . I would like to know whether a component is deterministic or not.

src/metric/metric_common.h Outdated Show resolved Hide resolved
src/metric/metric_common.h Outdated Show resolved Hide resolved
src/metric/metric_common.h Show resolved Hide resolved
src/metric/rank_metric.cc Show resolved Hide resolved
// Same thread can work on multiple groups one after another; hence, resize
// the predictions array based on the current group
rec.resize(gptr[group_id + 1] - gptr[group_id]);
#pragma omp parallel for schedule(static) reduction(+:total_pos, total_neg) \
Copy link
Member

Choose a reason for hiding this comment

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

We should add a test for non-omp build. I will work on this.

src/metric/rank_metric.cc Show resolved Hide resolved
@RAMitchell RAMitchell merged commit 5dc8e89 into dmlc:master Mar 3, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants