-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
There was a problem hiding this 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.
Errors are caused by FluxML/Zygote.jl#873. |
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
…tions.jl into st/doc_quickfixes
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
``` | ||
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`. |
There was a problem hiding this comment.
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))?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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]))
@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? |
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 If people feel that this is out of scope for this PR, I'll open a separate issue to discuss. |
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:
|
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 |
There was a problem hiding this 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.
There was a problem hiding this 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 😄
Yay! Happy to contribute my first PR on here. I've added points 1. and 2. to #221 to keep track of them. |
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.