Add check for variables in step samplers

[michaelraczycki]

What is this PR about?
Added safety check to prevent incorrect step assignment with custom step functions.
Nów assign_step_method from mcmc.py before assigning variables from step to
assigned_var, checks if they can be found in model.value_vars.

Major / Breaking Changes

New features

• step value check for assign_step_methods

Bugfixes

Documentation

Maintenance

[michaelraczycki]

contains feature requested in Issue #6511

[ricardoV94]

Looks good. I suggest a more informative error message. And we need a test, you can simplify the original issue on discourse and use that as test case.

[codecov]

Codecov Report

Merging #6524 (0849de9) into main (b9a3dcc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6524   +/-   ##
=======================================
  Coverage   91.96%   91.96%           
=======================================
  Files          89       89           
  Lines       14932    14935    +3     
=======================================
+ Hits        13732    13735    +3     
  Misses       1200     1200           

Impacted Files	Coverage Δ
pymc/sampling/mcmc.py	92.34% <100.00%> (+0.05%)	⬆️

[michaelraczycki]

@ricardoV94, I've noticed that most of the time the 'docs/readthedocs.org:pymc' test is failing, and the steps executed in it usually have very little to do with the PR's themselves. Have you considered splitting it into smaller steps or maybe increasing the time limit?

[ricardoV94]

@ricardoV94, I've noticed that most of the time the 'docs/readthedocs.org:pymc' test is failing, and the steps executed in it usually have very little to do with the PR's themselves. Have you considered splitting it into smaller steps or maybe increasing the time limit?

I don't think we have control over the time limit, it all happens on the ReadTheDocs side

[ricardoV94]

This PR needs a new test to show the check is working as expected

[michaelraczycki]

Is there something else that I should do here? If not then I could close the corresponding issue

[ricardoV94]

Is there something else that I should do here? If not then I could close the corresponding issue

Yes, you need to add a test that shows the new check is working as expected. That's always a requirement of pull requests, as it will help not changing the behavior inadvertently in the future.

[michaelraczycki]

@ricardoV94 So I have an idea to create the test in this way you can see in the commit, I'm not feeling very good with pytensor operations yet, so in case I'm misunderstanding anything please let me know. I'm not sure if the way I'd like to introduce a variable that's not in the model is correct, the test passes when I execute it locally. Feel free to suggest any changes!

[ricardoV94]

Looks good, some small tweak suggestions

[michaelosthege]

@michaelraczycki I rebase on latest main (the tests moved).

It looks like you committed with your user.email set to a local account name, which is why your name initially did not show up with the commits after I rebased them.

I edited the commit authors to the autogenerated GitHub user email..

Best update your git config --global user.email to an address that is configured under https://github.com/settings/emails