Skip to content

register LKJPrior if it's set.#748

Open
Jimbo994 wants to merge 1 commit intomainfrom
fix/lkj-prior
Open

register LKJPrior if it's set.#748
Jimbo994 wants to merge 1 commit intomainfrom
fix/lkj-prior

Conversation

@Jimbo994
Copy link
Copy Markdown
Collaborator

@Jimbo994 Jimbo994 commented Apr 4, 2026

Motivation

closes #745

now the LKJPrior is registered if it is specified as a task prior.

Have you read the Contributing Guidelines on pull requests?

Yes

Have you updated CHANGELOG.md?

Yes

Test Plan

Added tests.

@Jimbo994 Jimbo994 requested a review from jduerholt April 4, 2026 20:24
@Jimbo994
Copy link
Copy Markdown
Collaborator Author

Jimbo994 commented Apr 4, 2026

@jpfolch can you have a doublecheck if this correctly sets the LKJPrior? If yes @jduerholt can you review this?

Copy link
Copy Markdown
Contributor

@jduerholt jduerholt 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 to me in general, but do you know if the original issue is fixed? The one with the LKJ Prior mentioned in the comment?

@Jimbo994
Copy link
Copy Markdown
Collaborator Author

Jimbo994 commented Apr 7, 2026

Yes, so I now test that the LKJPrior is set, and test if the model is successfully fitted using model.fit, which uses fit_gpytorch_mll, and this works. So with that I assume it is fixed.

@jpfolch
Copy link
Copy Markdown
Contributor

jpfolch commented Apr 7, 2026

Here's the original issue which remains open, with some code snippets that claim to fix it here, but not sure if it was merged. Maybe it was fixed in some other PR but I can't find where exactly.

@Jimbo994
Copy link
Copy Markdown
Collaborator Author

Jimbo994 commented Apr 7, 2026

Ah, thanks for the context @jpfolch, I just checked, and it is indeed not solved/merged! If the model fits correctly on the first try, no error is thrown. If it doesn't, then sample_all_priors is called, and this still throws an error. When I ran the test I added 20 times, it sometimes fails the first fit and then indeed throws this error.

Let's maybe leave this PR open and keep an eye out for fixes in botorch.

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.

[Bug]: MultiTaskGPSurrogate task_prior is accepted but not applied (currently no-op)

3 participants