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

Fix Issue #4028: Fix ROC Evaluation Messages #4042

Merged
merged 3 commits into from Dec 23, 2017

Conversation

ckousik
Copy link
Contributor

@ckousik ckousik commented Dec 23, 2017

Changed the ROC Evaluation messages to the message format
described in the issue.

Changed the ROC Evaluation messages to the message format
described in the issue.
@@ -19,8 +19,10 @@ CROCEvaluation::~CROCEvaluation()

float64_t CROCEvaluation::evaluate(CLabels* predicted, CLabels* ground_truth)
{
REQUIRE(predicted->get_label_type()==LT_BINARY, "ROC evalution requires binary labels.");
REQUIRE(ground_truth->get_label_type()==LT_BINARY, "ROC evalution requires binary labels.");
REQUIRE(predicted->get_label_type()==LT_BINARY, "Given predicted labels (%d) must be binary (%d).",
Copy link
Member

@karlnapf karlnapf Dec 23, 2017

Choose a reason for hiding this comment

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

great thanks!

One thing. If one of the provided pointers is NULL ,this segfaults.
So you additionally need something like this for both arguments
REQUIRE(predicted, "No predicted labels provided\n.")

Copy link
Member

Choose a reason for hiding this comment

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

Also, you are missing a "\n" at the end of the error msg

@karlnapf
Copy link
Member

Thanks!
Needs minor fixing and then good to go.

Welcome to Shogun! :)

REQUIRE(predicted->get_label_type()==LT_BINARY, "ROC evalution requires binary labels.");
REQUIRE(ground_truth->get_label_type()==LT_BINARY, "ROC evalution requires binary labels.");
REQUIRE(predicted, "No predicted labels provided.\n");
REQUIRE(ground_truth, "No ground_truth labels provided.\n");
Copy link
Member

Choose a reason for hiding this comment

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

ground_truth -> ground truth

@karlnapf
Copy link
Member

Ace thanks!

One last thing (apart from my comment).
The style checker complains, https://travis-ci.org/shogun-toolbox/shogun/jobs/320637977#L745

Could you fix that? We recently are trying to get a more consistent style in

REQUIRE(ground_truth, "No ground_truth labels provided.\n");
REQUIRE(predicted->get_label_type()==LT_BINARY, "Given predicted labels (%d) must be binary (%d).\n",
predicted->get_label_type(), LT_BINARY);
REQUIRE(ground_truth->get_label_type()==LT_BINARY, "Given ground_truth labels (%d) must be binary (%d).\n",
Copy link
Member

Choose a reason for hiding this comment

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

ground_truth -> ground truth

@karlnapf
Copy link
Member

lovely, thanks! :)

@karlnapf karlnapf merged commit c412bee into shogun-toolbox:develop Dec 23, 2017
ktiefe pushed a commit to ktiefe/shogun that referenced this pull request Jul 30, 2019
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.

None yet

2 participants