Skip to content

/orange3

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

AUC / Rules #1750

Closed
wants to merge 4 commits into from

Conversation

Reviewers

@astaric astaric

@kernc kernc

Assignees

@matevzkren matevzkren

Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@martinmozina @CLAassistant @kernc @codecov-io @janezd @astaric @matevzkren
@martinmozina
Copy link
Contributor

commented Nov 13, 2016 •
edited by astaric

Issue
  1. Got wrong AUC values in domains with binary class
  2. Two small changes to rules.py
Description of changes

See commit messages.

Includes
  • Code changes
  • Tests
  • Documentation
@CLAassistant

This comment has been minimized.

Sign in to view
Copy link

commented Nov 13, 2016 •
edited

CLA assistant check
All committers have signed the CLA.

@kernc kernc assigned matevzkren Nov 14, 2016

@astaric

astaric reviewed Nov 18, 2016
View changes

Orange/tests/test_rules.py Outdated
@@ -36,12 +36,11 @@ def test_base_RuleLearner(self):
raised).
"""
base_rule_learner = _RuleLearner()
self.assertRaises(NotImplementedError, base_rule_learner.fit,
self.iris.X, self.iris.Y)
#self.assertRaises(NotImplementedError, base_rule_learner.fit,

This comment has been minimized.

Sign in to view
Copy link
@astaric

astaric Nov 18, 2016

Member

If the code will not be used any more, just remove these lines. git remembers what was here :)

This comment has been minimized.

Sign in to view
Copy link
@martinmozina

martinmozina Dec 2, 2016

Author Contributor

Fixed.
Orange/classification/rules.py Outdated
@@ -930,8 +929,8 @@ def __init__(self, preprocessors=None, base_rules=None):
self.cover_and_remove = self.exclusive_cover_and_remove
self.rule_stopping = self.lrs_significance_rule_stopping

def fit(self, X, Y, W=None):
raise NotImplementedError
#def fit_storage(self, data):

This comment has been minimized.

Sign in to view
Copy link
@astaric

astaric Nov 18, 2016

Member

If this code is not supposed to be here, just remove it

This comment has been minimized.

Sign in to view
Copy link
@martinmozina

martinmozina Dec 2, 2016

Author Contributor

Fixed.
Orange/evaluation/scoring.py Outdated
@@ -186,7 +186,10 @@ def compute_score(self, results, target=None):
if n_classes < 2:
raise ValueError("Class variable has less than two values")
elif n_classes == 2:
return self.from_predicted(results, skl_metrics.roc_auc_score)
return np.fromiter(

This comment has been minimized.

Sign in to view
Copy link
@astaric

astaric Nov 18, 2016

Member

A test fail on travis, probably caused by this change. You can see the error returned by test here:

https://travis-ci.org/biolab/orange3/jobs/175538053#L4091

This comment has been minimized.

Sign in to view
Copy link
@martinmozina

martinmozina Dec 2, 2016

Author Contributor

Fixed.
@kernc

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 2, 2016

The branch has conflicts. Can you rebase on the latest master?
@martinmozina

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Dec 3, 2016

Done. Never did that before, hope it is ok.
@codecov-io

This comment has been minimized.

Sign in to view
Copy link

commented Dec 3, 2016 •
edited

Current coverage is 88.99% (diff: 85.71%)

Merging #1750 into master will decrease coverage by 0.17%

@@             master      #1750   diff @@
==========================================
  Files            85         82     -3   
  Lines          9051       8965    -86   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           8070       7978    -92   
- Misses          981        987     +6   
  Partials          0          0

Sunburst

Powered by Codecov. Last update 7a745fe...872ed6a
@janezd

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Dec 9, 2016

@martinmozina, drop by our lab and somebody will help you removing the stray commits and resolve the conflict.

martinmozina added some commits Nov 13, 2016

@martinmozina
In compute_score of class AUC in case of two classes, roc_auc_score got 
actual predictions instead of predicted probabilities, which resulted in wrong
AUC.
dc00401
@martinmozina
Removed method fit from _RuleLearner, otherwise fit_storage is not ac… 
…cessible.

LRSValidator now tests both significances: vs parent rule and vs default rule.
0bd6134
@martinmozina
Fixed tests for AUC and rules. 
Simplified AUC calculation in scoring.py.
0cd7ace
@martinmozina
Sometimes rules should not be automatically evaluated. Added property 
where you can unset rule evaluation.
Loading status checks…
de6c8bf

@martinmozina martinmozina force-pushed the martinmozina:master branch from b7de21a to de6c8bf Dec 14, 2016

@kernc

kernc reviewed Dec 14, 2016
View changes

Orange/tests/test_evaluation_scoring.py
@@ -65,7 +65,7 @@ def test_call(self):

def test_bayes(self):
x = np.random.randint(2, size=(100, 5))
col = np.random.randint(5)
col = np.random.randint(5)

This comment has been minimized.

Sign in to view
Copy link
@kernc

kernc Dec 14, 2016

Member

This change errors.

This comment has been minimized.

Sign in to view
Copy link
@martinmozina

martinmozina Dec 16, 2016

Author Contributor

I fixed it, but nothing happened. I think the problem is that pull request is still from martinmozina:master branch, but should be from fix-auc branch. At least that's what we tried to achieve with @astaric.
@janezd

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Dec 16, 2016

Continued in #1826.

@janezd janezd closed this Dec 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.