-
Notifications
You must be signed in to change notification settings - Fork 74
recover_marginals infers model from model context #619
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
Conversation
6d6cdbb to
0dfb495
Compare
ricardoV94
left a 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.
Just some minor suggestions, let me know if too much of a drag
| """ | ||
| if isinstance(idata, Model): | ||
| raise TypeError("The first argument of `recover_marginals` must be an idata") |
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.
| raise TypeError("The first argument of `recover_marginals` must be an idata") | |
| raise TypeError("The order of arguments of `recover_marginals` changed. The first input must be an idata") |
|
No it's all fairly minor and happy to do so. I'm mainly just stuck on why
CI fails
…On Thu, 18 Dec 2025, 00:31 Ricardo Vieira, ***@***.***> wrote:
***@***.**** commented on this pull request.
Just some minor suggestions, let me know if too much of a drag
------------------------------
In pymc_extras/model/marginal/marginal_model.py
<#619 (comment)>
:
> idata: InferenceData,
+ model: Model | None = None,
Make all but idata kwarg only?
------------------------------
In pymc_extras/model/marginal/marginal_model.py
<#619 (comment)>
:
> @@ -389,6 +389,11 @@ def recover_marginals(
"""
+ if isinstance(idata, Model):
Add a comment that this is temporary check due to API change and should be
removed eventually
------------------------------
In pymc_extras/model/marginal/marginal_model.py
<#619 (comment)>
:
> @@ -389,6 +389,11 @@ def recover_marginals(
"""
+ if isinstance(idata, Model):
+ raise TypeError("The first argument of `recover_marginals` must be an idata")
⬇️ Suggested change
- raise TypeError("The first argument of `recover_marginals` must be an idata")
+ raise TypeError("The order of arguments of `recover_marginals` changed. The first input must be an idata")
------------------------------
In tests/model/marginal/test_marginal_model.py
<#619 (comment)>
:
> @@ -933,7 +936,7 @@ def test_nested(self):
)
idata = InferenceData(posterior=dict_to_dataset(prior))
- idata = recover_marginals(marginal_m, idata, return_samples=True)
+ idata = recover_marginals(idata, return_samples=True)
Are we testing the explicit model kwarg anywhere now?
—
Reply to this email directly, view it on GitHub
<#619 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACCUIITU4TOPFDO4CAKID4CHRVBAVCNFSM6AAAAACPEO2VISVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKOJQGAZTMNBRGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Need to ignore the arviz warning on project.toml and open an issue to address whatever it means. |
|
How do we ignore the arviz warning? I don't know how the project toml file
works?
…On Thu, 18 Dec 2025, 09:22 Ricardo Vieira, ***@***.***> wrote:
*ricardoV94* left a comment (pymc-devs/pymc-extras#619)
<#619 (comment)>
Need to ignore the arviz warning on project.toml and open an issue to
address whatever it means.
—
Reply to this email directly, view it on GitHub
<#619 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACCULXW7MY3VSJSV7GZDL4CJP27AVCNFSM6AAAAACPEO2VISVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMNRYHE4DSNZQGQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
It's done here: Lines 102 to 112 in 10d6765
|
|
Any idea why I'm getting this jax one?
https://github.com/pymc-devs/pymc-extras/actions/runs/20270017341/job/58203008839#step:5:535
…On Thu, 18 Dec 2025, 09:29 Ricardo Vieira, ***@***.***> wrote:
*ricardoV94* left a comment (pymc-devs/pymc-extras#619)
<#619 (comment)>
It's done here:
https://github.com/pymc-devs/pymc-extras/blob/10d6765ded4de2da9630e6f21540dcc8373da3e7/pyproject.toml#L102-L112
—
Reply to this email directly, view it on GitHub
<#619 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACCUMVNBCN72EWMIA62VT4CJQVXAVCNFSM6AAAAACPEO2VISVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMNRZGAZDKOJTGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Not because of any changes you did, so mark the test as xfail and open an issue to track? |
7a9d85e to
aec4360
Compare
7893941 to
126e31b
Compare
|
We have two RTD jobs? Anyway failure unlikely to be related |
|
Thanks @zaxtax |
This enables using
recover_marginalsin a model context. Just required making the model argument optionalCloses #610