Skip to content

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Oct 20, 2022

Closes #6223

I decided to not allow the old V3 behavior of passing intermediate variables/ Deterministic because I think that does something strange. Those variables are not the ones being sampled/maximized but instead the underlying input value variables are.

If a user wants to fit the variables that underlie a given graph, but he doesn't know what those are, they should do something like pm.find_MAP(vars=pm.inputvars(pm.aesaraf.rvs_to_value_vars(graph)))

Update: It is now allowed but a warning is emitted

Major / Breaking Changes

  • ...

Bugfixes / New features

  • ...

Docs / Maintenance

  • Check that only random variables are assigned to samplers and find_MAP

@ricardoV94 ricardoV94 force-pushed the check_sampler_value_vars branch 2 times, most recently from 2cb82ee to 2f2a139 Compare October 20, 2022 14:33
@ricardoV94
Copy link
Member Author

Why did the github bot remove the maintenance label?

@ricardoV94 ricardoV94 force-pushed the check_sampler_value_vars branch from 2f2a139 to 65e8454 Compare October 20, 2022 14:51
@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Merging #6235 (f66f320) into main (b5db350) will increase coverage by 2.32%.
The diff coverage is 94.59%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6235      +/-   ##
==========================================
+ Coverage   88.15%   90.47%   +2.32%     
==========================================
  Files         111      111              
  Lines       23820    23809      -11     
==========================================
+ Hits        20998    21542     +544     
+ Misses       2822     2267     -555     
Impacted Files Coverage Δ
pymc/aesaraf.py 94.04% <ø> (ø)
pymc/step_methods/metropolis.py 83.36% <85.71%> (-0.11%) ⬇️
pymc/tuning/starting.py 91.73% <91.66%> (-0.77%) ⬇️
pymc/model.py 89.53% <100.00%> (+0.29%) ⬆️
pymc/smc/kernels.py 97.37% <100.00%> (ø)
pymc/step_methods/hmc/base_hmc.py 89.90% <100.00%> (+0.09%) ⬆️
pymc/step_methods/slicer.py 96.15% <100.00%> (-0.05%) ⬇️
pymc/tests/tuning/test_starting.py 100.00% <100.00%> (ø)
pymc/backends/arviz.py 90.08% <0.00%> (+2.89%) ⬆️
... and 3 more

@ricardoV94 ricardoV94 force-pushed the check_sampler_value_vars branch from 4c0c16c to 5086972 Compare October 20, 2022 15:59
@ricardoV94 ricardoV94 requested a review from fonnesbeck October 20, 2022 16:41
@ricardoV94 ricardoV94 force-pushed the check_sampler_value_vars branch from 5086972 to 0d30225 Compare October 20, 2022 16:43
@ricardoV94
Copy link
Member Author

ricardoV94 commented Oct 21, 2022

This recent Discourse topic touches on the same issue of trying to pass something that's not an RV to a function that expects RVs: https://discourse.pymc.io/t/trying-to-use-pm-binarymetropolis/10645

With the changes in the PR the new error message would be (condensed example):

import pymc as pm
with pm.Model() as logistic_model_pred:
    p = pm.Beta("p", 1, 1)
    label = pm.Data("label", value=[0, 1, 2], mutable=True)
    observed = pm.Bernoulli("occupancy", p, observed=label)
    step = pm.BinaryMetropolis(vars=label)    
Traceback (most recent call last):
  File "/home/ricardo/miniconda3/envs/pymc/lib/python3.10/site-packages/IPython/core/interactiveshell.py", line 3397, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-2-f13ba6f7e166>", line 6, in <cell line: 2>
    step = pm.BinaryMetropolis(vars=label)
  File "/home/ricardo/Documents/Projects/pymc/pymc/step_methods/metropolis.py", line 395, in __init__
    vars = get_value_vars_from_user_vars(vars, model)
  File "/home/ricardo/Documents/Projects/pymc/pymc/util.py", line 403, in get_value_vars_from_user_vars
    raise ValueError(
ValueError: The following variables are not random variables in the model: ['label']

@ricardoV94 ricardoV94 requested a review from junpenglao October 21, 2022 13:39
@ricardoV94 ricardoV94 force-pushed the check_sampler_value_vars branch 4 times, most recently from 6a8ae88 to 86686c2 Compare October 21, 2022 14:41
@ricardoV94

This comment was marked as off-topic.

@ricardoV94 ricardoV94 force-pushed the check_sampler_value_vars branch from 86686c2 to f17836c Compare November 7, 2022 10:59
@ricardoV94 ricardoV94 requested a review from dfm November 7, 2022 11:05
@ricardoV94
Copy link
Member Author

Ready for review

@ricardoV94 ricardoV94 force-pushed the check_sampler_value_vars branch 2 times, most recently from 82af27e to 7295de4 Compare November 7, 2022 12:24
Copy link
Member

@junpenglao junpenglao left a comment

Choose a reason for hiding this comment

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

LGTM. Minor nit but feel free to ignore.

@ricardoV94 ricardoV94 force-pushed the check_sampler_value_vars branch from 7295de4 to 24e9545 Compare November 9, 2022 08:27
@ricardoV94 ricardoV94 force-pushed the check_sampler_value_vars branch from 24e9545 to 059ef47 Compare November 9, 2022 08:41
@ricardoV94 ricardoV94 force-pushed the check_sampler_value_vars branch from f548720 to f66f320 Compare November 9, 2022 11:33
@ricardoV94 ricardoV94 mentioned this pull request Nov 9, 2022
@ricardoV94
Copy link
Member Author

The MacOS job got stuck in conda-cache. Gonna take a risk and merge regardless

@ricardoV94 ricardoV94 merged commit 0be5295 into pymc-devs:main Nov 9, 2022
@ricardoV94 ricardoV94 deleted the check_sampler_value_vars branch June 6, 2023 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raise informative error when non-value variables are assigned to samplers
4 participants