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

Start fix for tranformer return shape #74

Merged
merged 3 commits into from
Sep 5, 2019

Conversation

ivirshup
Copy link
Contributor

@ivirshup ivirshup commented Sep 2, 2019

Fixes #73

Still needs a test, but I can add that tomorrow.

I've also reversed the dimensions specified by the .transform docs since a) I'm using the indices as they're returned and b) it fits the API of KNeighborsTransformer this way.

@lmcinnes
Copy link
Owner

lmcinnes commented Sep 2, 2019

Thanks, this looks promising. The whole KNeighborsTransformer thing has been in flux for quite a while so I wasn't paying too much attention, but it looks like it is almost ready to get merged so I guess it is time to make sure this meets the same API and does things right.

@ivirshup
Copy link
Contributor Author

ivirshup commented Sep 3, 2019

I think that should do it.

Related question, should the default settings be the same between NNDescent and PyNNDescentTransformer?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 103

  • 15 of 15 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 85.095%

Totals Coverage Status
Change from base Build 100: 0.2%
Covered Lines: 2061
Relevant Lines: 2422

💛 - Coveralls

@lmcinnes
Copy link
Owner

lmcinnes commented Sep 5, 2019

Thanks.

As for defaults being the same -- ideally. I believe there was some changes to the defaults for NNDescent, and those potentially never got migrated to the Transformer.

@lmcinnes lmcinnes merged commit 546f710 into lmcinnes:master Sep 5, 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.

PyNNDescentTransformer returns sparse matrix of wrong size
3 participants