-
-
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
Option to create non-shared pm.Data
#5295
Conversation
By passing `pm.Data(mutable=False)` one can create a `TensorConstant` instead of a `SharedVariable`. Data variables with known, fixed shape can enhance performance and compatibility in some situations. `pm.ConstantData` or `pm.MutableData` wrappers are provided as alternative syntax. This is the basis for solving pymc-devs#4441.
@ricardoV94 these changes could help for #4441 - I didn't test it, but with a (float) |
Codecov Report
@@ Coverage Diff @@
## main #5295 +/- ##
==========================================
+ Coverage 78.99% 79.06% +0.06%
==========================================
Files 87 87
Lines 14366 14394 +28
==========================================
+ Hits 11349 11380 +31
+ Misses 3017 3014 -3
|
Yes it should be possible, just need to consider the TensorConstant case in there |
In Also, should the default really be mutable=True? |
I'll make the error message more instructive 👍
No it shouldn't, but we can't flip that switch at the same time as we're introducing it. That's why I'd first default to showing Should I add the |
Sure, that all sounds good.
…On Mon, Jan 3, 2022, 16:33 Michael Osthege ***@***.***> wrote:
In pm.set_data() can we check if it was unmutable and then give an
informative error?
I'll make the error message more instructive 👍
Also, should the default really be mutable=True?
No it shouldn't, but we can't flip that switch at the same time as we're
introducing it. That's why I'd first default to showing pm.ConstantData/
pm.MutableDatain model graphs (see Slack).
Next step would be a FutureWarning.
Should I add the FutureWarning already? (When mutable is None..)
—
Reply to this email directly, view it on GitHub
<#5295 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFETGBEGUAXND3C2QC6SGTUUFUNXANCNFSM5K76572A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you commented.Message ID:
***@***.***>
|
d487cad
to
ea28e41
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.
Then we still need the check in pm.set_data()
if it was immutable.
It was there already, line 1132 in model.py |
👍 |
Until now
pm.Data
always created aSharedVariable
, even though most uses ofpm.Data
are not because it's mutable, but because it registers the variable in the model.With
pm.Data(..., mutable=False/True)
, this PR adds the option to create non-shared data variables (TensorConstant
) that possibly improve performance (via optimizations available forTensorConstant
) and can also be used in places where aSharedVariable
is not supported.I also refactored
pm.Data
from a class into a function. I'm not 100 % sure why it was a class in the first place, but if I understand it correctly our use of__new__
actually violates its intended signature, since we're not returning an instance of the correct type(?).Depending on what your PR does, here are a few things you might want to address in the description:
pm.Data.get_coord
staticmethod was refactored topm.data.determine_coords
, but this is just internal API.mutable
is not specified? We could also makemutable=None
by default and warn that the default will change in the future?Closes #5105