Correct the order of rvs sent to compile_dlogp in find_MAP
#5928
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ricardoV94
reviewed
Jun 25, 2022
pymc/tuning/starting.py
Outdated
| @@ -120,7 +120,8 @@ def find_MAP( | |||
| compiled_logp_func = DictToArrayBijection.mapf(model.compile_logp(jacobian=False), start) | |||
| logp_func = lambda x: compiled_logp_func(RaveledVars(x, x0.point_map_info)) | |||
|
|
|||
| rvs = [model.values_to_rvs[value] for value in vars] | |||
| vars_dict = {var.name: var for var in vars} | |||
There was a problem hiding this comment.
we could replace the var_names set above with this dict so that we don't create two objects with similar purposes.
|
Looks perfect! I left a small refactoring suggestion above, let me know what you think. |
quantheory
force-pushed
the
fix-find_MAP-vars-order
branch
from
5ff87c0 to
4970a76
Compare
|
Makes sense. I've made the change you suggested. |
Codecov Report
@@ Coverage Diff @@
## main #5928 +/- ##
=======================================
Coverage 89.52% 89.52%
=======================================
Files 73 73
Lines 13267 13267
=======================================
Hits 11877 11877
Misses 1390 1390
|
ricardoV94
approved these changes
Jun 25, 2022
|
Thanks @quantheory! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a quick fix for the bad behavior of
find_MAPin v4 (closes #5923).When using a gradient-based method in
scipy.minimize, we need the order of derivatives in the Jacobian to match the order of the variables inx. Sincex0.point_map_infois used to define this order in the rest of the function, I just use it to set the order of variables in thervsargument sent tocompile_dlogp, which fixes the issue.I've also added
test_find_MAP_issue_5923, which fails for the old code and passes in this commit. (In fact, the originaltest_find_MAPtest would also fail if we used a starting point slightly farther from the MAP.)