-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split convert observed data #7334
Split convert observed data #7334
Conversation
6b8e8f3
to
f8f554e
Compare
I'm still of the opinion we should drop generators altogether. I don't find any documentation on how to use it, and I can't really think of all the implications this Op means inside PyTensor, which is supposed to be side-effects free unless told otherwise (it's not being told so here). It's also only applicable in the default backend / and even here only if you have models without Deterministics (or any workflow where separate functions are compiled from the same underlying graph). If this is useful in VI, the machinery that creates and call functions can use regular Python code to work with generators (such as setting a shared variable to the next value in every iteration of the loop). I don't see why we need this magic at the PyTensor level. |
I'll add a depreaction warning to the conversion of generator data, asking people to get in touch if this is relevant. |
This will be used to fix the `GeneratorAdapter` when applied to generators producing int-valued data.
Previously, the `GeneratorAdapter` applied `floatX` to float data, but kept the original integer dtypes. `floatX` was then applied to everything by `convert_observed_data`. This refactor changes the handling of integer-valued generator data, such that `intX` is applied, and no `floatX` conversion takes place.
f8f554e
to
5a73475
Compare
|
pymc/pytensorf.py
Outdated
@@ -68,22 +68,38 @@ | |||
"floatX", | |||
"intX", | |||
"smartfloatX", | |||
"smarttypeX", |
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.
I would rather not add these new methods to all as I prefer we to get rid of them in the future, so the less discoverable they are the better.
For reference the whole intX is pretty questionable: #7331
876efc3
to
d9bd20c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7334 +/- ##
==========================================
- Coverage 92.47% 92.45% -0.03%
==========================================
Files 102 102
Lines 17188 17180 -8
==========================================
- Hits 15894 15883 -11
- Misses 1294 1297 +3
|
Description
convert_observed_data
is a messy do-it-all conversion function that was recently refactored in #7299.This PR takes the next step and splits it such that generator data is treated separately.
Related Issue
Checklist
Type of change
pm.Data
.📚 Documentation preview 📚: https://pymc--7334.org.readthedocs.build/en/7334/