Skip to content

[MRG] API: remove num_labeled parameter #119

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Jan 2, 2019
Merged

[MRG] API: remove num_labeled parameter #119

merged 17 commits into from
Jan 2, 2019

Conversation

wdevazelhes
Copy link
Member

Fixes #118

@wdevazelhes wdevazelhes changed the title API: remove num_labeled parameter [MRG] API: remove num_labeled parameter Aug 31, 2018
bellet
bellet approved these changes Aug 31, 2018
View reviewed changes
Copy link
Member

@bellet bellet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Maybe wait for @perimosocordiae to confirm that we did not miss an important use case for num_labeled

@terrytangyuan
Copy link
Member

We should probably record these kind of breaking changes in a change log?

@bellet
Copy link
Member

bellet commented Aug 31, 2018

I think the _Supervised classes are introduced in this next 0.4.0 release though?

@bellet
Copy link
Member

bellet commented Aug 31, 2018

My mistake it looks like they were introduced in 0.3.0 (albeit without doc)...
Then, maybe we can release 0.4.0 without this and save this PR for 0.5.0 (along with major API changes)?

Merge branch 'master' into fix/remove_num_labeled_parameter
0c0156f 
# Conflicts:
#	metric_learn/itml.py
#	metric_learn/lsml.py
#	metric_learn/mmc.py
#	metric_learn/sdml.py
perimosocordiae
perimosocordiae approved these changes Aug 31, 2018
View reviewed changes
Copy link
Contributor

@perimosocordiae perimosocordiae left a comment

Choose a reason for hiding this comment

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

Looks good. The original design was mostly for convenience when testing an algorithm: set num_labeled to 50% of the total, then score against the full set. It's not that useful for general users, so I'm happy to remove it.

I'll mark this for the 0.5 compatibility-breaking release.

@perimosocordiae perimosocordiae added this to the v0.5.0 milestone Aug 31, 2018
@wdevazelhes
Copy link
Member Author

I just added deprecation warnings in case we would want to put this in v0.4.0
But yes it makes also sense to put it in v0.5.0 where it would be part of lots of API changes
I'll update my commit replacing v0.4.0 by v.0.5.0 and v0.5.0 by 0.6.0
(Or maybe in v0.5.0 there will be so many changes that we shouldn't deprecate everything in the code so that it does not add too much complexity in it, and just list the changes in the changelog ?)

@bellet
Copy link
Member

bellet commented Sep 3, 2018 •
edited
Loading

For this a deprecation warning is probably good to have: it is simple (parameter is removed, default behavior does not change) and it is about the supervised metric learning algorithms (whose API should not change much in 0.5.0 except for this).

@bellet
Copy link
Member

bellet commented Jan 2, 2019

Except for the merge conflicts we should be able to merge this now?

Merge remote-tracking branch 'origin/master' into fix/remove_num_labe…
944bb3e 
…led_parameter

# Conflicts:
#	metric_learn/itml.py
#	metric_learn/lsml.py
#	metric_learn/mmc.py
@wdevazelhes
Copy link
Member Author

Yes I think so, I tried to resolve the conflicts but my branch was not updated so I messed it up :p I will try again...

William de Vazelhes added 5 commits January 2, 2019 15:52
Revert "MAINT: put deprecation for version 0.5.0"
bc1eb32 
This reverts commit 8727c44.
Revert "Merge remote-tracking branch 'origin/master' into fix/remove_…
d6bd0d4 
…num_labeled_parameter"

This reverts commit 944bb3e, reversing
changes made to 8727c44.
Revert "Revert "MAINT: put deprecation for version 0.5.0""
6691c1b 
This reverts commit bc1eb32.
Merge branch 'master' into fix/remove_num_labeled_parameter
e2e4169 
# Conflicts:
#	metric_learn/constraints.py
#	metric_learn/itml.py
#	metric_learn/lmnn.py
#	metric_learn/lsml.py
#	metric_learn/mmc.py
#	metric_learn/sdml.py
#	test/metric_learn_test.py
#	test/test_base_metric.py
FIX string representation test wrongly merged
83fef14
@terrytangyuan
Copy link
Member

Seems like rebase was done incorrectly/unsuccessfully? At least from a quick glance it seems like setup.py's changes are unexpected.

@wdevazelhes
Copy link
Member Author

Yes indeed, I think I did a bad revert, I'll revert try revert the revert d6bd0d4

@wdevazelhes
Copy link
Member Author

OK now there's just some tests for NCA that I don't understand why they are here...

@wdevazelhes
Copy link
Member Author

Oups, it appears I deleted it in the big merge #139 (cf. 23d0746#diff-1d435b5cb210c40bb6a7261667f5c5acL138)
I'll open an issue to put it back, so that it should disappear of the diff of this PR as soon as it is merged

@wdevazelhes
Copy link
Member Author

wdevazelhes commented Jan 2, 2019 •
edited
Loading

Here is the PR that we should merge (as soon as tests pass): #143

@wdevazelhes
Copy link
Member Author

I also checked in the diff of #139 for other wrong deletions and it was the only one

@wdevazelhes
Copy link
Member Author

OK in fact there are a few problems in #143, so I'll just delete the tests of NCA from here to be OK with the current master, and will deal with NCA's test separately in #143

@wdevazelhes
Copy link
Member Author

there just remains one error of version to update and it should be OK

@bellet
Copy link
Member

bellet commented Jan 2, 2019

still 1 failure ;-)

@wdevazelhes
Copy link
Member Author

Turns out I used the function Constraint.random_subset in #139 so I'll just need change those lines

@wdevazelhes
Copy link
Member Author

It should be good to merge now :p

@bellet bellet merged commit a9979a8 into scikit-learn-contrib:master Jan 2, 2019
@wdevazelhes wdevazelhes deleted the fix/remove_num_labeled_parameter branch January 3, 2019 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perimosocordiae perimosocordiae perimosocordiae approved these changes

@bellet bellet bellet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@wdevazelhes @terrytangyuan @bellet @perimosocordiae