8000 [MRG + 1] Fix repr on isotropic kernels when a 1-D length scale is given by MechCoder · Pull Request #7259 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

[MRG + 1] Fix repr on isotropic kernels when a 1-D length scale is given #7259

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

Merged
merged 4 commits into from
Sep 3, 2016

Conversation

MechCoder
Copy link
Member

What does this implement/fix? Explain your changes.

Moving setting the length_scale logic from the constructor to the methods broke __repr__ on a 1-D length_scale isotropic kernel.

 matern = Matern(length_scale=np.ones(1))
matern
TypeError: non-empty format string passed to object.__format__

This was because the formatting expected self.length_scale to be a scalar while the self.length_scale to be an array. To fix this, I squeeze the 1-D length_scale by squeezing it when __repr__ is called.

@amueller
Copy link
Member

Shouldn't we test all __repr__s? What's the test coverage on these? Was this not covered by a test or was the test not sufficient?

return "{0}(length_scale={1:.3g}, nu={2:.3g})".format(
self.__class__.__name__, self.length_scale, self.nu)
self.__class__.__name__, _squeeze(self.length_scale), self.nu)
Copy link
Member Author
@MechCoder MechCoder Aug 26, 2016

Choose a reason for hiding this comment

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

I reimplemented np.squeeze because .format does not play well with a 0-D array.

a = "{0:0.3g}".format(np.array(2))
TypeError: non-empty format string passed to object.__format__
a = "%0.3g" % np.array(2)
a
'2'

Probably should be reported to NumPy...

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

np.ravel(length_scale)[0] or np.atleast_1d(length_scale)[0] should also work. I think the helper function is a bit obscure to the reader.

@MechCoder
Copy link
Member Author

For now, I've just added a smoke test. Is that sufficient or should we test if stuff like "length_scale" is there in the string returned by repr?

Also ping @jnothman

@amueller
Copy link
Member

lgtm

@amueller amueller changed the title [MRG] Fix repr on isotropic kernels when a 1-D length scale is given [MRG + 1] Fix repr on isotropic kernels when a 1-D length scale is given Aug 31, 2016
@TomDLT
Copy link
Member
TomDLT commented Sep 1, 2016

I prefer np.ravel(length_scale)[0] to your _squeeze method.
Otherwise LG TM

@jnothman jnothman merged commit 49fb295 into scikit-learn:master Sep 3, 2016
@jnothman
Copy link
Member
jnothman commented Sep 3, 2016

Thanks

@MechCoder MechCoder deleted the fix_repr branch September 3, 2016 16:54
TomDLT pushed a commit to TomDLT/scikit-learn that referenced this pull request Oct 3, 2016
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
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.

4 participants
0