Make fit_pathfinder more similar to fit_laplace and pm.sample #447
Conversation
|
This looks alright to me. Thoughts @ricardoV94 ? |
|
CC @aphc14 |
pymc_extras/__init__.py
Outdated
| from pymc_extras.inference.find_map import find_MAP | ||
| from pymc_extras.inference.fit import fit | ||
| from pymc_extras.inference.laplace import fit_laplace | ||
| from pymc_extras.inference import * |
There was a problem hiding this comment.
Yes, of course. I'll fix that when I have a few minutes
| The inference data containing the results of the Pathfinder algorithm. | ||
|
|
||
| References | ||
| ---------- | ||
| Zhang, L., Carpenter, B., Gelman, A., & Vehtari, A. (2022). Pathfinder: Parallel quasi-Newton variational inference. Journal of Machine Learning Research, 23(306), 1-49. | ||
| """ | ||
|
|
||
| if initvals is not None: |
There was a problem hiding this comment.
Shouldn't mutate the model, make a copy perhaps if there's no better way to just forward the initvals
There was a problem hiding this comment.
I would normally agree. However, I tried it, but model.copy() does not produce a working model sometimes - most notably when any transformations are used.
Should I use some other copy function?
|
@ricardoV94 it currently also fails a test on including blackjax, which is currently included in pathfinder.py |
Yup, that is a good idea. closes #443 |
| The inference data containing the results of the Pathfinder algorithm. | ||
|
|
||
| References | ||
| ---------- | ||
| Zhang, L., Carpenter, B., Gelman, A., & Vehtari, A. (2022). Pathfinder: Parallel quasi-Newton variational inference. Journal of Machine Learning Research, 23(306), 1-49. | ||
| """ | ||
|
|
||
| if initvals is not None: | ||
| for rv_name, ivals in initvals.items(): | ||
| model.set_initval(model.named_vars[rv_name], ivals) |
There was a problem hiding this comment.
Might need to ensure that ivals is a support point for the RV. For example, x ~ Uniform(-1, 1) would have nan initial values with model.set_initval(model.named_vars["x"], 2)
There was a problem hiding this comment.
While in the ideal world, I would agree, in practice
a) It is very nontrivial to do as I understand, as the limits are not specified anywhere where they are easy to take
b) pm.sample does no such checks, and the goal of this PR is to be compatible with that
There was a problem hiding this comment.
Yup, seems fair enough. Thanks for this submission @velochy
|
Maybe we make a requirements-test.txt file like arviz has?
https://github.com/arviz-devs/arviz/blob/main/requirements-test.txt
…On Wed, 9 Apr 2025, 14:28 Margus Niitsoo, ***@***.***> wrote:
@ricardoV94 <https://github.com/ricardoV94> it currently also fails a
test on including blackjax, which is currently included in pathfinder.py
How would you like to handle it? Do I bring the include lower down so it
is only included when used, or do I add it to the requirements of the
package?
—
Reply to this email directly, view it on GitHub
<#447 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACCUM66WC34I7BYIQZWT32YUG5LAVCNFSM6AAAAAB2HVSQBWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOBZGUZTOMRUG4>
.
You are receiving this because you commented.Message ID:
***@***.***>
*velochy* left a comment (pymc-devs/pymc-extras#447)
<#447 (comment)>
@ricardoV94 <https://github.com/ricardoV94> it currently also fails a
test on including blackjax, which is currently included in pathfinder.py
How would you like to handle it? Do I bring the include lower down so it
is only included when used, or do I add it to the requirements of the
package?
—
Reply to this email directly, view it on GitHub
<#447 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACCUM66WC34I7BYIQZWT32YUG5LAVCNFSM6AAAAAB2HVSQBWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOBZGUZTOMRUG4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Considering you have a standing PR on this, does it not make more sense to just have your quickfix merged separately? |
|
Definitely!
…On Wed, 9 Apr 2025, 18:00 Margus Niitsoo, ***@***.***> wrote:
Yup, that is a good idea. closes #443
<#443>
Considering you have a standing PR on this, does it not make more sense to
just have your quickfix merged separately?
I don't think it conflicts with anything I've done here
—
Reply to this email directly, view it on GitHub
<#447 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACCUIEXAGQCBW56OPD6R32YU7ZZAVCNFSM6AAAAAB2HVSQBWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJQGIYDINJWHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
*velochy* left a comment (pymc-devs/pymc-extras#447)
<#447 (comment)>
Yup, that is a good idea. closes #443
<#443>
Considering you have a standing PR on this, does it not make more sense to
just have your quickfix merged separately?
I don't think it conflicts with anything I've done here
—
Reply to this email directly, view it on GitHub
<#447 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACCUIEXAGQCBW56OPD6R32YU7ZZAVCNFSM6AAAAAB2HVSQBWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOOJQGIYDINJWHE>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
…ue for pathfinder and cleaned relevant docs a bit
velochy
force-pushed
the
FitCompatibility
branch
from
1ee9bcf to
370ffe0
Compare
|
The latest test failure is due to pathfinder using |
This arose out of https://discourse.pymc.io/t/pymc-extras-fit-compatibility-with-pm-sample/16763/5