Bring back sampler argument target_accept #5622

aloctavodia · 2022-03-19T12:56:31Z

Edit:
This also fixes #4291 by enforcing init to be a string

codecov · 2022-03-19T13:22:24Z

Codecov Report

Merging #5622 (aa20d7e) into main (8b063f9) will decrease coverage by 18.26%.
The diff coverage is 66.66%.

❗ Current head aa20d7e differs from pull request most recent head 1e4a1ac. Consider uploading reports for the commit 1e4a1ac to get more accurate results

@@             Coverage Diff             @@
##             main    #5622       +/-   ##
===========================================
- Coverage   87.67%   69.40%   -18.27%     
===========================================
  Files          76       76               
  Lines       13717    13724        +7     
===========================================
- Hits        12026     9525     -2501     
- Misses       1691     4199     +2508

Impacted Files	Coverage Δ
pymc/sampling.py	`50.65% <66.66%> (-35.44%)`	⬇️
pymc/step_methods/mlda.py	`12.53% <0.00%> (-83.85%)`	⬇️
pymc/tuning/starting.py	`19.51% <0.00%> (-73.18%)`	⬇️
pymc/step_methods/slicer.py	`26.76% <0.00%> (-69.02%)`	⬇️
pymc/variational/inference.py	`19.38% <0.00%> (-67.86%)`	⬇️
pymc/parallel_sampling.py	`20.59% <0.00%> (-66.12%)`	⬇️
pymc/func_utils.py	`22.50% <0.00%> (-65.00%)`	⬇️
pymc/variational/test_functions.py	`38.09% <0.00%> (-61.91%)`	⬇️
pymc/step_methods/hmc/hmc.py	`33.96% <0.00%> (-58.50%)`	⬇️
pymc/variational/stein.py	`48.33% <0.00%> (-51.67%)`	⬇️
... and 30 more

pymc/sampling.py

ricardoV94 · 2022-03-19T13:25:07Z

Can we add a regression test. Perhaps a short sampling and the checking the sample_stats target_accept?

aloctavodia · 2022-03-19T15:56:50Z

We have a test with the argument init=None here

pymc/pymc/tests/test_quadpotential.py

Line 153 in c3ffec8

pymc.sample(10, init=None, step=step, chains=1)

but we have a condition here

pymc/pymc/sampling.py

Lines 2262 to 2263 in 849a64d

    
           if not isinstance(init, str): 
        
               raise TypeError("init must be a string.")

restricting init to be a string (which agrees with the docstrings)

ricardoV94 · 2022-03-19T15:59:07Z

I had opened an issue about that long time ago: #4291

I would say we raise if it's not a string

michaelosthege

I would recommend to bring back this entire section: 2dd4c8c#diff-7b3470589153c7c90a2c05fc86402f9acf65555200d90f01b51a47ccef592628L484-L506

The reason is mostly that before attempting to do a at.grad there's no way to know if the model is differentiable. The old try/catch logic took care of that.

pymc/sampling.py

pymc/tests/test_sampling.py

ricardoV94

We should merge this soon, but is there a test that ensures we are doing the automatic init?

pymc/sampling.py

ricardoV94 · 2022-03-21T19:02:08Z

We should release immediately after this gets in. @aloctavodia Did you confirm we don't do any useful NUTS init when we get a default CompoundStep? If so, we should open an issue for that

aloctavodia · 2022-03-21T19:09:35Z

Agreed, and yes we should open an issue.

ricardoV94 · 2022-03-21T19:10:01Z

Agreed, and yes we should open an issue.

Do you mind doing that, as you have more context atm?

aloctavodia · 2022-03-21T19:11:22Z

Yes, sure!

michaelosthege · 2022-03-21T19:12:38Z

pymc/sampling.py

+    elif isinstance(step, NUTS) and auto_nuts_init:
+        if "nuts" in kwargs:
+            nuts_kwargs = kwargs.pop("nuts")
+            [kwargs.setdefault(k, v) for k, v in nuts_kwargs.items()]
+        _log.info("Auto-assigning NUTS sampler...")
+        initial_points, step = init_nuts(
+            init=init,
+            chains=chains,
+            n_init=n_init,
+            model=model,
+            seeds=random_seed,
+            progressbar=progressbar,
+            jitter_max_retries=jitter_max_retries,
+            tune=tune,
+            initvals=initvals,
+            **kwargs,
+        )


I'm not confident about this. As I commented before, the try/catch block caught not-implemented gradients, which we're probably not testing.

Also I'm sceptical about the pattern of having isinstance(step, NUTS) and if "nuts" in kwargs. It reads like a hack to account for some specific cases, and it's not as general as the previous implementation.

You're the ones who've been looking into this much more than me, so feel free to overrule this.

The assign_step_methods checks for gradients as well. If that does not do a good enough job of testing if NUTS is appropriate then it should be treated as a bug and be fixed there

Okay, you're right. Also we're testing it: https://app.codecov.io/gh/pymc-devs/pymc/blob/18346acc797c32ccfa2d61ebc8f368ef03bc216c/pymc/sampling.py

aloctavodia requested a review from ricardoV94 March 19, 2022 12:56

ricardoV94 reviewed Mar 19, 2022

View reviewed changes

pymc/sampling.py Outdated Show resolved Hide resolved

aloctavodia force-pushed the steps branch from 3f1ae28 to aab25b3 Compare March 19, 2022 15:20

michaelosthege requested changes Mar 19, 2022

View reviewed changes

pymc/sampling.py Outdated Show resolved Hide resolved

aloctavodia force-pushed the steps branch from aa20d7e to 69aec42 Compare March 21, 2022 07:48

ricardoV94 reviewed Mar 21, 2022

View reviewed changes

pymc/tests/test_sampling.py Outdated Show resolved Hide resolved

aloctavodia force-pushed the steps branch 3 times, most recently from a7e66c4 to 1b5a666 Compare March 21, 2022 11:07

ricardoV94 reviewed Mar 21, 2022

View reviewed changes

pymc/sampling.py Outdated Show resolved Hide resolved

revert remove init_nuts

1e4a1ac

aloctavodia force-pushed the steps branch from 1b5a666 to 1e4a1ac Compare March 21, 2022 18:20

ricardoV94 approved these changes Mar 21, 2022

View reviewed changes

ricardoV94 requested a review from michaelosthege March 21, 2022 19:00

michaelosthege reviewed Mar 21, 2022

View reviewed changes

michaelosthege approved these changes Mar 21, 2022

View reviewed changes

ricardoV94 merged commit 5b31ec7 into pymc-devs:main Mar 21, 2022

aloctavodia deleted the steps branch March 22, 2022 05:15

ricardoV94 mentioned this pull request Mar 28, 2022

init_nuts not used when CompoundStep is used #5658

Closed

ricardoV94 mentioned this pull request Aug 16, 2024

Default target_accept not mentioned anywhere in the docs #7455

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring back sampler argument target_accept #5622

Bring back sampler argument target_accept #5622

aloctavodia commented Mar 19, 2022 •

edited

Loading

codecov bot commented Mar 19, 2022 •

edited

Loading

ricardoV94 commented Mar 19, 2022 •

edited

Loading

aloctavodia commented Mar 19, 2022

ricardoV94 commented Mar 19, 2022

michaelosthege left a comment

ricardoV94 left a comment

ricardoV94 commented Mar 21, 2022 •

edited

Loading

aloctavodia commented Mar 21, 2022

ricardoV94 commented Mar 21, 2022

aloctavodia commented Mar 21, 2022

michaelosthege Mar 21, 2022

ricardoV94 Mar 21, 2022 •

edited

Loading

michaelosthege Mar 21, 2022

Bring back sampler argument target_accept #5622

Bring back sampler argument target_accept #5622

Conversation

aloctavodia commented Mar 19, 2022 • edited Loading

codecov bot commented Mar 19, 2022 • edited Loading

Codecov Report

ricardoV94 commented Mar 19, 2022 • edited Loading

aloctavodia commented Mar 19, 2022

ricardoV94 commented Mar 19, 2022

michaelosthege left a comment

Choose a reason for hiding this comment

ricardoV94 left a comment

Choose a reason for hiding this comment

ricardoV94 commented Mar 21, 2022 • edited Loading

aloctavodia commented Mar 21, 2022

ricardoV94 commented Mar 21, 2022

aloctavodia commented Mar 21, 2022

michaelosthege Mar 21, 2022

Choose a reason for hiding this comment

ricardoV94 Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

michaelosthege Mar 21, 2022

Choose a reason for hiding this comment

aloctavodia commented Mar 19, 2022 •

edited

Loading

codecov bot commented Mar 19, 2022 •

edited

Loading

ricardoV94 commented Mar 19, 2022 •

edited

Loading

ricardoV94 commented Mar 21, 2022 •

edited

Loading

ricardoV94 Mar 21, 2022 •

edited

Loading