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

Rework the precision metric. #9222

Merged
merged 13 commits into from
Jun 2, 2023
Merged

Rework the precision metric. #9222

merged 13 commits into from
Jun 2, 2023

Conversation

trivialfis
Copy link
Member

I just realized this was not a documented feature before the PR.

  • Rework the precision metric for both CPU and GPU.
  • Mention it in the document.
  • Clean up old support code for GPU ranking metric.

The new implementation supports only learning to rank. Please consider one of the existing library implementations like sklearn for classification tasks. The previous implementation claims support for binary classification in a comment, which is false.

It is extracted from #8822.

trivialfis added 12 commits May 31, 2023 03:12
- Rework the precision metric for both CPU and GPU.
- Mention it in document.
- Cleanup old support code for GPU ranking metric.
- Add proper support for binary classification.
@@ -372,12 +372,37 @@ bool IsBinaryRel(linalg::VectorView<float const> label, AllOf all_of) {
* both CPU and GPU.
*/
template <typename AllOf>
void CheckMapLabels(linalg::VectorView<float const> label, AllOf all_of) {
void CheckPreLabels(StringView name, linalg::VectorView<float const> label, AllOf all_of) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update the docstring description: \brief Validate label for the Precision metric.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Comment on lines -4 to -22
// When device ordinal is present, we would want to build the metrics on the GPU. It is *not*
// possible for a valid device ordinal to be present for non GPU builds. However, it is possible
// for an invalid device ordinal to be specified in GPU builds - to train/predict and/or compute
// the metrics on CPU. To accommodate these scenarios, the following is done for the metrics
// accelerated on the GPU.
// - An internal GPU registry holds all the GPU metric types (defined in the .cu file)
// - An instance of the appropriate GPU metric type is created when a device ordinal is present
// - If the creation is successful, the metric computation is done on the device
// - else, it falls back on the CPU
// - The GPU metric types are *only* registered when xgboost is built for GPUs
//
// This is done for 2 reasons:
// - Clear separation of CPU and GPU logic
// - Sorting datasets containing large number of rows is (much) faster when parallel sort
// semantics is used on the CPU. The __gnu_parallel/concurrency primitives needed to perform
// this cannot be used when the translation unit is compiled using the 'nvcc' compiler (as the
// corresponding headers that brings in those function declaration can't be included with CUDA).
// This precludes the CPU and GPU logic to coexist inside a .cu file

Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is no longer valid?

Copy link
Member Author

Choose a reason for hiding this comment

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

The GPU registry is removed in this PR, as the precision metric is the last one being rewritten.

Comment on lines 321 to 323
<< "Invalid size of weight. For a binary classification task, it's size should be equal "
"to the number of samples. For a learning to rank task, it's size should be equal to "
"the number of query groups.";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought the Precision metric is only used for the learning-to-rank task? If so, we should not mention the binary classification task from this error message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed. This comment didn't get cleaned up. I tried to support binary classification at one point but gave up as the existing implementations are feature rich.

@trivialfis trivialfis merged commit 9fbde21 into dmlc:master Jun 2, 2023
@trivialfis trivialfis deleted the metric-pre branch June 2, 2023 12:49
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.

2 participants