-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Adding meta-information for MeasurableOps #7076
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7076 +/- ##
===========================================
- Coverage 92.30% 63.23% -29.08%
===========================================
Files 100 100
Lines 16895 16990 +95
===========================================
- Hits 15595 10743 -4852
- Misses 1300 6247 +4947
|
pymc/logprob/abstract.py
Outdated
*args, | ||
ndim_supp: Union[int, Tuple[int]], | ||
supp_axes: Tuple[Union[int, Tuple[int]]], | ||
measure_type: Union[MeasureType, Tuple[MeasureType]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not that be a frozenset or some collection? Union requires if else later, does not it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what you mean in more detail?
1f21516
to
0a55eb5
Compare
@Dhruvanshu-Joshi do you have a chance to update the PR to fix the conflicts? Otherwise was anything missing? |
No there was not any feature missing to be added. I'll solve the conflicts and push the updates asap. |
0a55eb5
to
7157900
Compare
7157900
to
ddcda2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment. Otherwise looks good to me. Pre-commit seems to be unhappy though
if len(base_var.owner.outputs) != len(base_op.ndim_supp): | ||
raise NotImplementedError("length of outputs and meta-properties is different") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too restrictive for Scan, which can have recurrent outputs that are not measurable variables. Let's just remove it for now?
Description
This PR aims to solve issue #6360 and is a cotinuation of the PR #6685 and PR #6754 by incorporating RV meta information in intermediate MeasurableVariables. The Measurable ops covered are MeasurableComparison, MeasurableClip, MeasurableRound, MeasurableSpecifyShape, MeasurableCheckAndRaise, MeasurableIfElse, MeasurableScan, MeasurableMakeVector, MeasurableJoin, MeasurableDimShuffle, MeasurableTransforms and DiracDelta.
cc: @ricardoV94 @larryshamalama
Related Issue
Checklist
Type of change
📚 Documentation preview 📚: https://pymc--7076.org.readthedocs.build/en/7076/