Skip to content

Conversation

Michal-Novomestsky
Copy link
Contributor

@Michal-Novomestsky Michal-Novomestsky commented Jul 2, 2025

Addresses #532 and #344.

Relies on pymc-devs/pytensor#1582.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Michal-Novomestsky Michal-Novomestsky force-pushed the implement-pmx.fit-option-for-INLA-+-marginalisation-routine branch from 398979a to c6010f3 Compare August 12, 2025 11:05
@Michal-Novomestsky Michal-Novomestsky force-pushed the implement-pmx.fit-option-for-INLA-+-marginalisation-routine branch from dad163c to a473e87 Compare August 12, 2025 11:12
@Michal-Novomestsky
Copy link
Contributor Author

@maresb, @zaxtax suggested that I reach out to you about the Docs not being built. Do you have any ideas why it's failing? Thanks!

Copy link

@ColtAllen ColtAllen left a comment

Choose a reason for hiding this comment

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

Just docstring suggestions for now.

*args,
Q: TensorVariable,
minimizer_seed: int,
minimizer_kwargs: dict = {"method": "L-BFGS-B", "optimizer_kwargs": {"tol": 1e-8}},

Choose a reason for hiding this comment

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

This may create unpredictable behavior if any changes are made to the minimizer kwargs. Let's make None the default argument and add a conditional for the default kwargs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I coded it up like that originally but Theo had the exact opposite opinion haha, thoughts @theorashid ?


# logp(y | x, params)
log_likelihood = pt.sum(
[logp_term.sum() for value, logp_term in logps_dict.items() if value is not marginalized_vv]

Choose a reason for hiding this comment

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

pt.switch could work for this I think.

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.

6 participants