/scikit-learn

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

[MRG] Deprecate lsh forest #9078

Merged
merged 3 commits into from Jun 10, 2017

Conversation

Reviewers

@amueller amueller

@ogrisel ogrisel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@ldirer @ogrisel @amueller
@ldirer
Contributor

ldirer commented Jun 9, 2017

Reference Issue

Fixes #8996

What does this implement/fix? Explain your changes.

Add a deprecation (removed in 0.21) and performance warning for LSHForest.

Any other comments?

  • I could use help on a better message for performance warning ;).
  • Should we put the performance warning message in the deprecation message?
    Since both are linked it would make sense I think. For now I issued 2 separate warnings.
  • I'm puzzled by the @ignore_warnings decorator which does not seem to be doing his job in nested function calls: I had to use inline calls to ignore_warnings everywhere, which is not super nice.

@ldirer ldirer changed the title from Deprecate lsh forest to [WIP] Deprecate lsh forest Jun 9, 2017

Show outdated Hide outdated sklearn/neighbors/approximate.py
@@ -217,6 +217,10 @@ def __init__(self, n_estimators=10, radius=1.0, n_candidates=50,
self.min_hash_match = min_hash_match
self.radius_cutoff_ratio = radius_cutoff_ratio
warnings.warn("LSHForest has poor performance.")

This comment has been minimized.

Show comment
Hide comment
@ogrisel

ogrisel Jun 9, 2017

Member

Please remove that line. Or making an inline comment instead of an independent UserWarning.
Copy URL
@ogrisel

ogrisel Jun 9, 2017 •
edited
Edited 1 time
  • @ogrisel ogrisel edited Jun 9, 2017 (most recent)

Member

Please remove that line. Or making an inline comment instead of an independent UserWarning.

This comment has been minimized.

Show comment
Hide comment
@ldirer

ldirer Jun 9, 2017

Contributor

The original issue mentioned

LSHForest should be deprecated and scheduled for removal in 0.21. It should also warn about having bad performance

I could make the deprecation warning:

    LSHForest has poor performance and has been deprecated in 0.19. It will be removed in version 0.21.
Copy URL
@ldirer

ldirer Jun 9, 2017

Contributor

The original issue mentioned

LSHForest should be deprecated and scheduled for removal in 0.21. It should also warn about having bad performance

I could make the deprecation warning:

    LSHForest has poor performance and has been deprecated in 0.19. It will be removed in version 0.21.
@ogrisel

This comment has been minimized.

Show comment
Hide comment
@ogrisel

ogrisel Jun 9, 2017

Member

Remaining TODOs:

  • remove any reference to LSHForest in the narrative documentation, API documentation and examples.
  • update whats_new.rst to mention the deprecation.
Member

ogrisel commented Jun 9, 2017 •
edited
Edited 1 time
  • @ogrisel ogrisel edited Jun 9, 2017 (most recent)

Remaining TODOs:

  • remove any reference to LSHForest in the narrative documentation, API documentation and examples.
  • update whats_new.rst to mention the deprecation.
@ldirer

This comment has been minimized.

Show comment
Hide comment
@ldirer

ldirer Jun 9, 2017

Contributor

I removed the section on approximate neighbors entirely, but I'm having second thoughts about it.
I think it's informative (beyond what's implemented in scikit-learn).

If we go ahead and remove it I'll mention in the relevant PRs (that allow ANN, I think PR #8999 does that) that what we removed can be used as a basis for new documentation.

I would have liked to keep some paragraphs but it feels weird to have general considerations that don't relate to any scikit-learn class.
Contributor

ldirer commented Jun 9, 2017

I removed the section on approximate neighbors entirely, but I'm having second thoughts about it.
I think it's informative (beyond what's implemented in scikit-learn).

If we go ahead and remove it I'll mention in the relevant PRs (that allow ANN, I think PR #8999 does that) that what we removed can be used as a basis for new documentation.

I would have liked to keep some paragraphs but it feels weird to have general considerations that don't relate to any scikit-learn class.

@ldirer ldirer changed the title from [WIP] Deprecate lsh forest to [MRG] Deprecate lsh forest Jun 9, 2017

Show outdated Hide outdated doc/whats_new.rst
@@ -427,6 +427,10 @@ API changes summary
:class:`cluster.bicluster.SpectralBiclustering` now accept ``y`` in fit.
:issue:`6126` by `Andreas Müller`_.
- :class:`neighbors.approximate.LSHForest` has been deprecated and will be
removed in 0.21 due to poor performance.
:issue:`8996` by `Andreas Müller`_.

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Jun 9, 2017

Member

the by refers to the person coding, not the issue raiser
Copy URL
@amueller

amueller Jun 9, 2017

Member

the by refers to the person coding, not the issue raiser
@ogrisel

ogrisel approved these changes Jun 9, 2017
View changes

Let's also delete benchmarks/bench_plot_approximate_neighbors.py and +1 on my side.

@ldirer

This comment has been minimized.

Show comment
Hide comment
@ldirer

ldirer Jun 10, 2017

Contributor

Appveyor seems to have randomly failed but otherwise I think it's ready for merge.
Contributor

ldirer commented Jun 10, 2017

Appveyor seems to have randomly failed but otherwise I think it's ready for merge.
@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Jun 10, 2017

Member

LGTM, thanks
Member

amueller commented Jun 10, 2017

LGTM, thanks

@amueller amueller merged commit 2a1410b into scikit-learn:master Jun 10, 2017
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

@ldirer @Sundrique
[MRG] Deprecate lsh forest (#9078) 
* Add deprecation message and test.

* Adding performance warning and ignore_warnings in test

* Add deprecation to whatsnew and remove LSHForest references from docs.

Removing benchmark for lsh
32c9489

@bmanohar16 bmanohar16 referenced this pull request Jul 20, 2017

Closed

Fix remaining sphinx errors #9417

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

@ldirer @dmohns
[MRG] Deprecate lsh forest (#9078) 
* Add deprecation message and test.

* Adding performance warning and ignore_warnings in test

* Add deprecation to whatsnew and remove LSHForest references from docs.

Removing benchmark for lsh
7cd6190

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

@ldirer @dmohns
[MRG] Deprecate lsh forest (#9078) 
* Add deprecation message and test.

* Adding performance warning and ignore_warnings in test

* Add deprecation to whatsnew and remove LSHForest references from docs.

Removing benchmark for lsh
ffa4eb7

NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

@ldirer @NelleV
[MRG] Deprecate lsh forest (#9078) 
* Add deprecation message and test.

* Adding performance warning and ignore_warnings in test

* Add deprecation to whatsnew and remove LSHForest references from docs.

Removing benchmark for lsh
324bc62

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

@ldirer @paulha
[MRG] Deprecate lsh forest (#9078) 
* Add deprecation message and test.

* Adding performance warning and ignore_warnings in test

* Add deprecation to whatsnew and remove LSHForest references from docs.

Removing benchmark for lsh
ea995da

AishwaryaRK added a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017

@ldirer @AishwaryaRK
[MRG] Deprecate lsh forest (#9078) 
* Add deprecation message and test.

* Adding performance warning and ignore_warnings in test

* Add deprecation to whatsnew and remove LSHForest references from docs.

Removing benchmark for lsh
fe702b6

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

@ldirer @maskani-moh
[MRG] Deprecate lsh forest (#9078) 
* Add deprecation message and test.

* Adding performance warning and ignore_warnings in test

* Add deprecation to whatsnew and remove LSHForest references from docs.

Removing benchmark for lsh
7b491c3

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

@ldirer Jeremiah Johnson
[MRG] Deprecate lsh forest (#9078) 
* Add deprecation message and test.

* Adding performance warning and ignore_warnings in test

* Add deprecation to whatsnew and remove LSHForest references from docs.

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