Skip to content

Update GP ARD docs to use built-in cov function#858

Merged
bob-carpenter merged 1 commit intostan-dev:masterfrom
amas0:update-gp-docs
Jan 28, 2025
Merged

Update GP ARD docs to use built-in cov function#858
bob-carpenter merged 1 commit intostan-dev:masterfrom
amas0:update-gp-docs

Conversation

@amas0
Copy link
Copy Markdown
Contributor

@amas0 amas0 commented Jan 28, 2025

Submission Checklist

  • Builds locally
  • New functions marked with <<{ since VERSION }>>
  • Declare copyright holder and open-source license: see below

Summary

Slight rewrite of the Automatic Relevance Determination section the Stan User's guide on Gaussian processes.

Proposed to fix #856

@bob-carpenter
Copy link
Copy Markdown
Member

Thanks for opening a PR.

I'm sorry I wasn't clear before.

We try very hard in the User's Guide to present fairly optimal code. If we use less efficient code, our users will copy it and be unhappy.

It is OK to provide a less efficient alternative for pedagogical purposes before introducing the more efficient variant. Here, you can leave the Cholesky-based version, then note that various pieces add up to the same thing as doing it directly with a dense covariance matrix rather than its Cholesky factor.

Copy link
Copy Markdown
Member

@bob-carpenter bob-carpenter left a comment

Choose a reason for hiding this comment

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

My last comment was in the wrong place. Sorry for being misleading before, but we do not want to remove the efficient version of this with Cholesky factors.

What I would recommend is adding a note that shows the equivalent dense covariance matrix parameterization and mentions it is less efficient than working with Cholesky factors.

We do not want to provide code that users copy that isn't efficient.

@amas0
Copy link
Copy Markdown
Contributor Author

amas0 commented Jan 28, 2025 via email

@bob-carpenter
Copy link
Copy Markdown
Member

Thanks, @amas0. My bad. I didn't see that the L_... form wasn't a built-in but was something the user built from scratch with their own squared exponential kernel.

I'll go back and approve now that I see what's going on!

Copy link
Copy Markdown
Member

@bob-carpenter bob-carpenter left a comment

Choose a reason for hiding this comment

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

Looks good---I missed the first time that the L_gp_exp_quad_cov_ARD was user-defined!

@bob-carpenter bob-carpenter merged commit 31454c2 into stan-dev:master Jan 28, 2025
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.

Should User's Guide page on Gaussian Process ARD use the built-in covariance function?

2 participants