8000 [MRG] ENH test isinstance(.., GPKernel) only if module pre-loaded by jnothman · Pull Request #7930 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG] ENH test isinstance(.., GPKernel) only if module pre-loaded #7930

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
Closed
wants to merge 3 commits into from
Closed

[MRG] ENH test isinstance(.., GPKernel) only if module pre-loaded #7930

wants to merge 3 commits into from

Conversation

jnothman
Copy link
Member

No description provided.

@jnothman
Copy link
Member Author

TODO: confirm test coverage of both lines 91 and 93 in sklearn/utils/metaestimators.py

@amueller
Copy link
Member

test failures. Do you feel like the import is slow or why do you want to make it conditional?

@amueller
Copy link
Member

You could also move the import into the if test so that it's only imported if there is a kernel attribute which is unlikely?

@jnothman
Copy link
Member Author
jnothman commented Nov 30, 2016 •
edited
Loading

I hope I've fixed the CI.

Well, I think there are multiple bad things about the current design:

  • importing implementation (non-util) code from utils risks circular import problems in user code. This is a small issue fixed by this PR.
  • we shouldn't really be special-casing on the basis of isinstance. It's a red flag that if a user had implemented our GP module privately, it would break here in grid searches. This is a bigger issue I've not set my mind to.

@jnothman
Copy link
Member Author

Sorry, got confused. Making the import local already fixed that small issue. Yes, this code only really fixes an issue of import runtime costs (1 per job in parallel). I'm ambivalent about it.

@lesteve
Copy link
Member
lesteve commented Dec 1, 2016

I'd vote to close unless the import is known to be particularly slow or importing it has a side-effect like loading a big .so in memory.

@jnothman jnothman closed this Dec 2, 2016
@amueller
Copy link
Member
amueller commented Dec 6, 2016

This is a bit of a red flag indeed. I'm not sure if the condition maybe predates the existence of _pairwise. There is not really a reason to have the condition there, as far as I can see - unless a user implements something that has a kernel and doesn't specify _pairwise properly.

@jnothman
Copy link
Member Author
jnothman commented Dec 7, 2016

_pairwise is only True when the kernel (by whatever name) is 'precomputed'. I don't see how this affects the permissibility of custom kernel functions. Using a non-precomputed custom kernel is disallowed, though I don't really get why, except to reduce unnecessary computation.

@amueller
Copy link
Member
amueller commented Dec 7, 2016

Right. Now I understand even less why this is there... Should we remove it?

@jnothman
Copy link
Member Author
jnothman commented Dec 7, 2016 via email

@amueller
Copy link
Member
amueller commented Dec 7, 2016

@jnothman no. It does predate the _precomputed fix, and I really don't understand what it's doing. Removing the lines doesn't make any tests fail...

@amueller amueller mentioned this pull request Dec 7, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jnothman @amueller @lesteve
0