Fix issue with pickling Deterministic #4120
Conversation
Spaak
changed the title
|
@AlexAndorra @canyon289 @MarcoGorelli @aloctavodia @ahartikainen With e9bc36c, this PR should fix #4112 while keeping all new |
Codecov Report
@@ Coverage Diff @@
## master #4120 +/- ##
=======================================
Coverage 88.75% 88.75%
=======================================
Files 89 89
Lines 14040 14041 +1
=======================================
+ Hits 12461 12462 +1
Misses 1579 1579
|
| new_type = type(old_type.__name__ + '_pymc3_Deterministic', (old_type,), | ||
| {'__str__': functools.partial(_repr_deterministic_rv, var, formatting='plain')}) | ||
| var.__class__ = new_type | ||
| var.__class__ = DeterministicWrapper # adds str and latex functionality |
There was a problem hiding this comment.
Is it possible for var to be a numpy array here*, in which case, would assigning its .__class__ attribute to a subclass of tt.TensorVariable cause problems? (I'm new to PyMC3 so sorry for the basic question :) )
*the definition of floatX would suggest it
There was a problem hiding this comment.
This will indeed cause errors if var is not a TensorVariable. I think a deterministic will always be a TensorVariable, at least it is in all the test scripts and demos. It's a good question, though; I'm not sure there is any formal specification requiring that it has to be. (But I cannot really see how a numpy-typed deterministic makes sense, given that all model factors are TensorVariables anyway (and all results of e.g. numpy * tensorvariable etc. are also of type TensorVariable))
|
This looks great to me. Thank you so much for the fast follow up |
This should fix #4112 in a more specific way than #4117. Work in progress, but filing now already to share the relevant
strtest. The fix itself should be done later today.