Create arrow to observation nodes subject to arbitrary dtype casting in pm.model_to_graphviz
#6011
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6011 +/- ##
==========================================
+ Coverage 89.15% 89.17% +0.01%
==========================================
Files 72 72
Lines 12895 12900 +5
==========================================
+ Hits 11496 11503 +7
+ Misses 1399 1397 -2
|
|
@larryshamalama Would be great to get this one over the finish line for the new release! |
larryshamalama
force-pushed
the
fix-graphviz-obs
branch
from
b7cbce9
to
102c124
Compare
| @@ -236,6 +256,10 @@ class TestUnnamedObservedNodes(BaseModelGraphTest): | |||
| model_func = model_unnamed_observed_node | |||
|
|
|||
|
|
|||
| class TestObservationDtypeCasting(BaseModelGraphTest): | |||
| model_func = model_observation_dtype_casting | |||
There was a problem hiding this comment.
Why not just add the function here into the class directly?
There was a problem hiding this comment.
I just naively followed the structure of previous tests based on BaseModelGraphTest...
There was a problem hiding this comment.
Thanks, I see that now that I expanded past diffs. Dont worry about it on this PR, ill just openly say I dont believe this is the right pattern, it very much feels like we should be using fixtures to reference model setup blocks as pytest provides many more functionality and guarantees when we use its architecture correctly
Thanks for the PR!
There was a problem hiding this comment.
I'm not super familiar with the alternative approach that you're proposing. Maybe it could be something to improve at a later date. In short, what do you mean by using fixtures and why does it allow better usage of pytest? Is this page what you are referring to? I'm going to guess that the answer to this is beyond the scope of this PR and can warrant a whole discussion...
|
Thanks @larryshamalama for providing this. Before we merge can we rebase on main as well so it gets tested by our 3.7 test runners that just got added? |
|
Need a moment to fix another bug, marking this as draft temporarily Edit: fixed! With this PR merged, @drbenvincent's notebook should display the right graph |
larryshamalama
force-pushed
the
fix-graphviz-obs
branch
from
0addee6
to
f6dbbfe
Compare
larryshamalama
changed the title
ConstantData observation nodes in pm.model_to_graphvizdtype casting in pm.model_to_graphviz
Closes #5795.
As described in the issue, when casting data to another
dtype, a casting operation is performed and ignored by thepm.model_to_graphvizfunctionality.However, without investigating further, I believe that this PR covers the scope of #5795, but there is an important example of another problem which seems to be caused by a different issue. I hope to have a look at the latter this Wednesday.
CC @drbenvincent