Rename "labels" to "classes" fixes Issue #1591 #1692

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
4 participants
@hrishikeshio
Contributor

hrishikeshio commented Feb 19, 2013

Done for clusters/
except for kmeans. I need advice.
The _kmeans.py has lots of functions/attributs/variables with "label" in it.
Should those all be deprecated?
Passes all the tests but I also I need to update the tests to make sure it is giving deprecation warnings.
fixes Issue #1591

@hrishikeshio hrishikeshio referenced this pull request Feb 19, 2013

Closed

deprecated numpy api #1250

@amueller

This comment has been minimized.

Show comment Hide comment
@amueller

amueller Feb 19, 2013

Owner

I haven't looked at kmeans but it is not so important to rename all variables. Things that are visible to the users such as parameters and attributes are the important ones.

I'll try to have a look at kmeans if I find the time.

Owner

amueller commented Feb 19, 2013

I haven't looked at kmeans but it is not so important to rename all variables. Things that are visible to the users such as parameters and attributes are the important ones.

I'll try to have a look at kmeans if I find the time.

@amueller

This comment has been minimized.

Show comment Hide comment
@amueller

amueller Feb 19, 2013

Owner

This looks good so far btw :)

Owner

amueller commented Feb 19, 2013

This looks good so far btw :)

@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Feb 20, 2013

Contributor

Thanks @amueller . What about the pyx files? Do I need to change them? Never worked with these before..
Also, what is the best way to handle warnings?

Ideally I think you would leave the old tests in, catch the deprecation warnings and add new tests. That way we know we are backward compatible and also have tests for the new behavior.
Contributor

hrishikeshio commented Feb 20, 2013

Thanks @amueller . What about the pyx files? Do I need to change them? Never worked with these before..
Also, what is the best way to handle warnings?

Ideally I think you would leave the old tests in, catch the deprecation warnings and add new tests. That way we know we are backward compatible and also have tests for the new behavior.
@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Feb 20, 2013

Contributor
@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Feb 20, 2013

Contributor

Please check the latest commit with deprecation warning testing. is it ok?

Contributor

hrishikeshio commented Feb 20, 2013

Please check the latest commit with deprecation warning testing. is it ok?

@amueller

This comment has been minimized.

Show comment Hide comment
@amueller

amueller Feb 20, 2013

Owner

the last commit is exactly right. You will see the same pattern in other tests. (git grep catch).
For cython: I guess it depends on whether there are any public functions. which are you thinking about? (sorry I'm on a very tight schedule atm).

Owner

amueller commented Feb 20, 2013

the last commit is exactly right. You will see the same pattern in other tests. (git grep catch).
For cython: I guess it depends on whether there are any public functions. which are you thinking about? (sorry I'm on a very tight schedule atm).

@jaquesgrobler

This comment has been minimized.

Show comment Hide comment
@jaquesgrobler

jaquesgrobler Feb 20, 2013

Owner

The warning tests look good for me. Same way I've done it previously.

Owner

jaquesgrobler commented Feb 20, 2013

The warning tests look good for me. Same way I've done it previously.

@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Feb 20, 2013

Contributor

Clusters/ is completed. Also added deprecation warning test and updated all test files there.

Contributor

hrishikeshio commented Feb 20, 2013

Clusters/ is completed. Also added deprecation warning test and updated all test files there.

@amueller

This comment has been minimized.

Show comment Hide comment
@amueller

amueller Feb 20, 2013

Owner

Do you want this reviewed / merged now or do you want to go on with the other modules? Looks good so far imho.

Owner

amueller commented Feb 20, 2013

Do you want this reviewed / merged now or do you want to go on with the other modules? Looks good so far imho.

@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Feb 21, 2013

Contributor

I think I will do it one module at a time. So that I know I am on right track at all times. I am a believer in Lean methodology (Sorry if it makes me a little annoying :P)
So, Yes. I think we should review and merge this.
Also it might be easy to track a problem to a merge maybe, if problems arise in future.

Contributor

hrishikeshio commented Feb 21, 2013

I think I will do it one module at a time. So that I know I am on right track at all times. I am a believer in Lean methodology (Sorry if it makes me a little annoying :P)
So, Yes. I think we should review and merge this.
Also it might be easy to track a problem to a merge maybe, if problems arise in future.

@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Feb 21, 2013

Contributor

I did some git grepping. It seems clusters was only module that needed change. Other classifiers are not using it.
So this is a complete merge I guess.

Contributor

hrishikeshio commented Feb 21, 2013

I did some git grepping. It seems clusters was only module that needed change. Other classifiers are not using it.
So this is a complete merge I guess.

@amueller

This comment has been minimized.

Show comment Hide comment
@amueller

amueller Feb 21, 2013

Owner

i'm pretty sure the metrics module uses it.

Hrishikesh Huilgolkar notifications@github.com schrieb:

I did some git grepping. It seems clusters was only module that needed
change. Other classifiers are not using it.
So this is a complete merge I guess.


Reply to this email directly or view it on GitHub:
#1692 (comment)

Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

Owner

amueller commented Feb 21, 2013

i'm pretty sure the metrics module uses it.

Hrishikesh Huilgolkar notifications@github.com schrieb:

I did some git grepping. It seems clusters was only module that needed
change. Other classifiers are not using it.
So this is a complete merge I guess.


Reply to this email directly or view it on GitHub:
#1692 (comment)

Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Feb 21, 2013

Contributor

Ok I will look into it.

Contributor

hrishikeshio commented Feb 21, 2013

Ok I will look into it.

@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Feb 21, 2013

Contributor

Done for metrics.

Contributor

hrishikeshio commented Feb 21, 2013

Done for metrics.

@amueller

This comment has been minimized.

Show comment Hide comment
@amueller

amueller Feb 22, 2013

I'm afraid that needs to be backward compatible, too.

I'm afraid that needs to be backward compatible, too.

@amueller

This comment has been minimized.

Show comment Hide comment
@amueller

amueller Feb 22, 2013

Owner

Thanks, great. Have you had a look at the tests for the metrics, too?

Owner

amueller commented Feb 22, 2013

Thanks, great. Have you had a look at the tests for the metrics, too?

labels deprecated in some more files.
All tests pass. pep8 fixed
@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Feb 23, 2013

Contributor

All tests pass for metrics. I will add warning checks in tests now.

Contributor

hrishikeshio commented Feb 23, 2013

All tests pass for metrics. I will add warning checks in tests now.

@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Feb 23, 2013

Contributor

Added tests for metrics. LGTM for merge 👍

Contributor

hrishikeshio commented Feb 23, 2013

Added tests for metrics. LGTM for merge 👍

@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Feb 27, 2013

Contributor

Guys can we please merge this?

Contributor

hrishikeshio commented Feb 27, 2013

Guys can we please merge this?

@ogrisel

This comment has been minimized.

Show comment Hide comment
@ogrisel

ogrisel Feb 27, 2013

Owner

Before merging the examples need to be updated, for instance I get a warning:

$ python examples/cluster/plot_affinity_propagation.py 

=================================================
Demo of affinity propagation clustering algorithm
=================================================

Reference:
Brendan J. Frey and Delbert Dueck, "Clustering by Passing Messages
Between Data Points", Science Feb. 2007


/Users/ogrisel/coding/scikit-learn/sklearn/utils/__init__.py:89: DeprecationWarning: Function labels_ is deprecated; Attribute labels_ is deprecated and will be removed in 0.15. Use 'classes_' instead
  warnings.warn(msg, category=DeprecationWarning)
Estimated number of clusters: 3
Homogeneity: 0.872
Completeness: 0.872
V-measure: 0.872
Adjusted Rand Index: 0.912
Adjusted Mutual Information: 0.871
Silhouette Coefficient: 0.753

There are many other occurrences of labels_:

$ git grep labels_
Owner

ogrisel commented Feb 27, 2013

Before merging the examples need to be updated, for instance I get a warning:

$ python examples/cluster/plot_affinity_propagation.py 

=================================================
Demo of affinity propagation clustering algorithm
=================================================

Reference:
Brendan J. Frey and Delbert Dueck, "Clustering by Passing Messages
Between Data Points", Science Feb. 2007


/Users/ogrisel/coding/scikit-learn/sklearn/utils/__init__.py:89: DeprecationWarning: Function labels_ is deprecated; Attribute labels_ is deprecated and will be removed in 0.15. Use 'classes_' instead
  warnings.warn(msg, category=DeprecationWarning)
Estimated number of clusters: 3
Homogeneity: 0.872
Completeness: 0.872
V-measure: 0.872
Adjusted Rand Index: 0.912
Adjusted Mutual Information: 0.871
Silhouette Coefficient: 0.753

There are many other occurrences of labels_:

$ git grep labels_
@amueller

This comment has been minimized.

Show comment Hide comment
@amueller

amueller Mar 3, 2013

Owner

I guess we can merge without renaming all the other labels_ as a first step. We should get rid of the deprecation warning, though.

Owner

amueller commented Mar 3, 2013

I guess we can merge without renaming all the other labels_ as a first step. We should get rid of the deprecation warning, though.

@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Mar 3, 2013

Contributor

Yeah. I don't want this PR to take too much time and become stagnant. Also am pretty busy with life ATM so renaming all labels_ will take time.
So what can I do to make this PR merge @amueller ? I can update the examples instead of removing the warnings.. I guess that will be easier..

Contributor

hrishikeshio commented Mar 3, 2013

Yeah. I don't want this PR to take too much time and become stagnant. Also am pretty busy with life ATM so renaming all labels_ will take time.
So what can I do to make this PR merge @amueller ? I can update the examples instead of removing the warnings.. I guess that will be easier..

@ogrisel

This comment has been minimized.

Show comment Hide comment
@ogrisel

ogrisel Mar 3, 2013

Owner

I can update the examples instead of removing the warnings.. I guess that will be easier..

I think updating the examples is precisely what @amueller had in mind when he said to remove the warnings.

Owner

ogrisel commented Mar 3, 2013

I can update the examples instead of removing the warnings.. I guess that will be easier..

I think updating the examples is precisely what @amueller had in mind when he said to remove the warnings.

@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Mar 3, 2013

Contributor

Oh. I will get this done this week.

Contributor

hrishikeshio commented Mar 3, 2013

Oh. I will get this done this week.

@ogrisel

This comment has been minimized.

Show comment Hide comment
@ogrisel

ogrisel Mar 3, 2013

Owner

Thanks!

Owner

ogrisel commented Mar 3, 2013

Thanks!

@hrishikeshio

This comment has been minimized.

Show comment Hide comment
@hrishikeshio

hrishikeshio Mar 4, 2013

Contributor
Contributor

hrishikeshio commented Mar 4, 2013

@ogrisel

This comment has been minimized.

Show comment Hide comment
@ogrisel

ogrisel Mar 4, 2013

Owner

I did a quick visual check of the diff and it looks good be someone should check that the doc still builds fine with the updated examples:

cd doc
make clean html
Owner

ogrisel commented Mar 4, 2013

I did a quick visual check of the diff and it looks good be someone should check that the doc still builds fine with the updated examples:

cd doc
make clean html
@amueller

This comment has been minimized.

Show comment Hide comment
@amueller

amueller Mar 9, 2013

Owner

I'm running the tests and will merge then.

Owner

amueller commented Mar 9, 2013

I'm running the tests and will merge then.

@amueller

This comment has been minimized.

Show comment Hide comment
@amueller

amueller Mar 9, 2013

Owner

wait... damn...
this is not right :-/
Sorry, I was not specific enough / didn't think it through.
The labels_ attribute in cluster does something very different from the classes_ attribute for the classification algorithms. Giving the two the same name would be very bad.
Sorry you did this work :-( What I meant in the original issue was to replace all the instances of label that are used in the supervised algorithms. Sorry I didn't pay more attention here.

Owner

amueller commented Mar 9, 2013

wait... damn...
this is not right :-/
Sorry, I was not specific enough / didn't think it through.
The labels_ attribute in cluster does something very different from the classes_ attribute for the classification algorithms. Giving the two the same name would be very bad.
Sorry you did this work :-( What I meant in the original issue was to replace all the instances of label that are used in the supervised algorithms. Sorry I didn't pay more attention here.

@amueller

This comment has been minimized.

Show comment Hide comment
@amueller

amueller Jan 9, 2015

Owner

Closing this as now labels_ is public api for clustering.

Owner

amueller commented Jan 9, 2015

Closing this as now labels_ is public api for clustering.

@amueller amueller closed this Jan 9, 2015

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