Skip to content
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

Enforce custom initval in SMC #7439

Merged
merged 3 commits into from
Aug 1, 2024
Merged

Conversation

tvwenger
Copy link
Contributor

@tvwenger tvwenger commented Jul 29, 2024

Description

Per the discussion here, it is apparent that SMC does not respect custom initval. This is a problem when a custom initval is required in order to support a transformation, such as Ordered.

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

📚 Documentation preview 📚: https://pymc--7439.org.readthedocs.build/en/7439/

tests/smc/test_smc.py Outdated Show resolved Hide resolved
pymc/smc/kernels.py Outdated Show resolved Hide resolved
pymc/smc/kernels.py Outdated Show resolved Hide resolved
@tvwenger
Copy link
Contributor Author

@ricardoV94 thanks for the review! Before I revert the formatting changes (thanks VSCode!), I want to address the comment

Won't this override the default behavior of sampling from the prior, to using the moment?

I guess this is true, and we'll need a different strategy for this fix! If you have any ideas, please let me know.

@ricardoV94
Copy link
Member

@ricardoV94 thanks for the review! Before I revert the formatting changes (thanks VSCode!), I want to address the comment

Won't this override the default behavior of sampling from the prior, to using the moment?

I guess this is true, and we'll need a different strategy for this fix! If you have any ideas, please let me know.

Ordered transform is an edge case that keeps posing problems all over the place. Much of this pain would be gone with: #7297

@ricardoV94
Copy link
Member

What is model.rvs_to_initvals when those are not specified though? I'm not sure the default is moment, my comment was a question not a statement

@tvwenger
Copy link
Contributor Author

What is model.rvs_to_initvals when those are not specified though? I'm not sure the default is moment, my comment was a question not a statement

Thanks for the clarification @ricardoV94 ! Indeed, the default value for model.rvs_to_initvals is None unless initval is set. Thus, sampling from the prior distribution will be respected unless initval is set. I've updated the documentation to make this explicit!

I've also reverted those pesky formatting changes.

Copy link

codecov bot commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.18%. Comparing base (8eaa9be) to head (439bffd).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7439   +/-   ##
=======================================
  Coverage   92.18%   92.18%           
=======================================
  Files         103      103           
  Lines       17263    17263           
=======================================
  Hits        15914    15914           
  Misses       1349     1349           
Files Coverage Δ
pymc/smc/kernels.py 98.16% <ø> (ø)

@ricardoV94 ricardoV94 added maintenance SMC Sequential Monte Carlo labels Jul 30, 2024
@aloctavodia aloctavodia merged commit b407c01 into pymc-devs:main Aug 1, 2024
22 checks passed
@aloctavodia
Copy link
Member

Thanks @tvwenger!

@tvwenger tvwenger deleted the smc_ordered branch August 1, 2024 17:52
@tvwenger
Copy link
Contributor Author

tvwenger commented Aug 2, 2024

@aloctavodia Actually, I think this "fix" causes other issues. When sampling a model with sample_smc and this change, I get errors like:

Traceback (most recent call last):
  File "/home/twenger/miniconda3/envs/bayes_spec-dev/lib/python3.12/concurrent/futures/process.py", line 263, in _process_worker
    r = call_item.fn(*call_item.args, **call_item.kwargs)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/twenger/science/python/pymc/pymc/smc/sampling.py", line 352, in _sample_smc_int
    smc.mutate()
  File "/home/twenger/science/python/pymc/pymc/smc/kernels.py", line 428, in mutate
    pearson_r = corr.get(self.tempered_posterior)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/twenger/science/python/pymc/pymc/smc/kernels.py", line 466, in get
    return np.abs(ab / (self.aa * bb))

Presumably, these errors are due to the fact that all of the starting positions are equal, thus there is no variance in the starting particle positions and the denominator of this pearson_r derivation is zero. Yikes!

@jessegrabowski had another idea here that I will try to implement...

@aloctavodia
Copy link
Member

Ohh, I see. Thanks for keep trying to find a good solution

@ricardoV94
Copy link
Member

@aloctavodia the PR should have been squashed+merge, since it had useless / badly titled commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance SMC Sequential Monte Carlo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants