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

Documentation improvements (inc. lengthscale explanation) and Matern12Kernel alias #213

Merged
merged 20 commits into from
Jan 9, 2021

Conversation

st--
Copy link
Member

@st-- st-- commented Jan 6, 2021

Just some minor edits for clarity and typos while going through the docs, and adds Matern12Kernel as alias for ExponentialKernel (in line with the explicitly defined Matern32Kernel and Matern52Kernel). Incorporates the lengthscales explanation from #212.

@st-- st-- requested a review from theogf January 6, 2021 18:44
Copy link
Member

@theogf theogf left a comment

Choose a reason for hiding this comment

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

It looks good, I don't see any issue. You don't necessarily have to correct spacing in latex since it is going to be ignored anyway.

docs/src/kernels.md Outdated Show resolved Hide resolved
docs/src/kernels.md Outdated Show resolved Hide resolved
docs/src/metrics.md Outdated Show resolved Hide resolved
docs/src/transform.md Outdated Show resolved Hide resolved
docs/src/transform.md Outdated Show resolved Hide resolved
@devmotion
Copy link
Member

Errors are caused by FluxML/Zygote.jl#873.

docs/src/kernels.md Outdated Show resolved Hide resolved
docs/src/kernels.md Outdated Show resolved Hide resolved
@st-- st-- changed the title Minor improvements to the documentation Minor improvements to the documentation; add Matern12Kernel alias Jan 7, 2021
docs/src/kernels.md Outdated Show resolved Hide resolved
docs/src/kernels.md Outdated Show resolved Hide resolved
@devmotion

This comment has been minimized.

docs/src/kernels.md Outdated Show resolved Hide resolved
@st--

This comment has been minimized.

@theogf

This comment has been minimized.

docs/src/kernels.md Outdated Show resolved Hide resolved
docs/src/userguide.md Outdated Show resolved Hide resolved
```
One can get the array of parameters to optimize via `params` from `Flux.jl`

One can access the named tuple of trainable parameters via `Functors.functor` from `Functors.jl`.
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we explicitly mention using first(Functors.functor(k)) as an example (see #212 (comment))?

Copy link
Member

Choose a reason for hiding this comment

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

This seems like a good idea to me -- a brief sentence would probably suffice. @devmotion @theogf any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

So I checked and this is not completely true unfortunately:

using Functors
k = 2.5 * transform(SqExponentialKernel(), 2.0)
first(functor(k)) == ((kernel = Squared Exponential Kernel
	- Scale Transform (s = 2.0), σ² = [2.5]))

@st-- st-- changed the title Documentation improvements and Matern12Kernel alias Documentation improvements (inc. lengthscale explanation) and Matern12Kernel alias Jan 8, 2021
@st--
Copy link
Member Author

st-- commented Jan 9, 2021

@theogf @willtebbutt @devmotion I've addressed all review comments as far as I can tell, though I've got a few questions left open. Other than that, what else is missing for you to approve this PR?

@willtebbutt
Copy link
Member

willtebbutt commented Jan 9, 2021

Thanks for the contribution @st-- . I'm happy for this to be merged as-is, but I'm also happy to discuss / address the following in this PR or another.

I'm concerned is that we're duplicating documentation a bit at the minute by having both docstrings and having the entire Kernels section written out manually. This creates an opportunity for the docs to decouple from the docstrings and increases the maintenance burden. I wonder whether it would make sense, therefore, to consolidate this documentation -- if we were to move the concise mathematical description of the kernels from the docs to the docstrings, and just move /duplicate bits of the API section in the Kernel Functions section we could get the best of both worlds?

If people feel that this is out of scope for this PR, I'll open a separate issue to discuss.

@st--
Copy link
Member Author

st-- commented Jan 9, 2021

I've opened #217 and #218 to discuss/address some of the points that were brought up by this PR. I'd be in favour of keeping PRs small (this one went from just fixing a few typos to taking several days already 😅 ), and split out these larger discussion/change points.

Two open questions:

  1. Documentation improvements (inc. lengthscale explanation) and Matern12Kernel alias #213 (comment): Is that a good(-enough) way of incorporating TensorProduct for now? (And/)Or should we open another issue for that?
  2. Documentation improvements (inc. lengthscale explanation) and Matern12Kernel alias #213 (comment): Should we explicitly mention using first(Functors.functor(k)) as an example (see #212 (comment))?

@devmotion
Copy link
Member

I'm fine with the PR, I think further improvements (in particular, updating the docstrings and using them on the webpage) should be done in other PRs. I also think 1 and 2 should not be addressed in this PR - I think it is better to not mention TensorProduct there for now instead of forcing it into this section which only mentions sums and products, and in my opinion most users won't call or look into Functors.functor, so we might not need an example there (currently).

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

Looks like I forgot to hit approve.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Ah I also forgot it 😄

@st-- st-- mentioned this pull request Jan 9, 2021
3 tasks
@st--
Copy link
Member Author

st-- commented Jan 9, 2021

Yay! Happy to contribute my first PR on here. I've added points 1. and 2. to #221 to keep track of them.

@st-- st-- merged commit 11008d6 into master Jan 9, 2021
@st-- st-- deleted the st/doc_quickfixes branch January 9, 2021 17:05
@devmotion devmotion mentioned this pull request Jan 9, 2021
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