- Notifications
You must be signed in to change notification settings - Fork2.2k
Add warning if a Minibatched variable is used without total_size#7742
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ttenThis should help catch a well-known Minibatch footgun
codecovbot commentedMar 30, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #7742 +/- ##======================================= Coverage 92.82% 92.82% ======================================= Files 107 107 Lines 18324 18326 +2 =======================================+ Hits 17010 17012 +2 Misses 1314 1314
🚀 New features to boost your workflow:
|
jessegrabowski left a comment
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.
LGTM. My only suggestion is that we have some way for a user who knows he doesn't want to settotal_size to somehow avoid the warning. I can't think of a case where anyone would want to do this, but something liketotal_size=False to silence the warning might be nice?
Might also be too convoluted.
zaxtax commentedApr 1, 2025 via email• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I don't think you ever want to not set total_size if you are doingminibatching. If you are doing something fancy you probably are justsetting it to something other than len(data) …On Tue, 1 Apr 2025, 10:10 Jesse Grabowski, ***@***.***> wrote: ***@***.**** approved this pull request. LGTM. My only suggestion is that we have some way for a user who knows he doesn't want to set total_size to somehow avoid the warning. I can't think of a case where anyone would want to do this, but something like total_size=False to silence the warning might be nice? Might also be too convoluted. — Reply to this email directly, view it on GitHub <#7742 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACCUIZBJRM5CAGC5PTEQL2XJCWHAVCNFSM6AAAAAB2CSI7Q6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOMZRHA2TGMBXGI> . You are receiving this because you authored the thread.Message ID: ***@***.***> |
4185b7b intopymc-devs:mainUh oh!
There was an error while loading.Please reload this page.
| self.add_named_variable(rv_var,dims) | ||
| self.set_initval(rv_var,initval) | ||
| else: | ||
| if ( |
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 should have checked if minibatch is anywhere in the ancestors of value not just immediately
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.
Do we need to do that? This check is only for observed random variables. I thought for those we don't allow the value to depend on another node
…c-devs#7742)Add warning if a Minibatched variable is used but total_size is forgottenThis should help catch a well-known Minibatch footgun
Uh oh!
There was an error while loading.Please reload this page.
Description
This should help catch a well-known Minibatch footgun where a user forgets to include the
total_sizeargumentChecklist
Type of change
📚 Documentation preview 📚:https://pymc--7742.org.readthedocs.build/en/7742/