-
Notifications
You must be signed in to change notification settings - Fork 244
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
Add basis bumps effect to MMM model #1475
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1475 +/- ##
==========================================
+ Coverage 92.55% 92.59% +0.04%
==========================================
Files 52 52
Lines 5947 6026 +79
==========================================
+ Hits 5504 5580 +76
- Misses 443 446 +3 ☔ View full report in Codecov by Sentry. |
def create_basis_matrix(s_ref, e_ref): | ||
return pt.where( | ||
(s_ref >= 0) & (e_ref <= 0), | ||
0, | ||
pt.where(pt.abs(s_ref) < pt.abs(e_ref), s_ref, e_ref), |
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 want control over this? @juanitorduz
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.
seems reasonable
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.
Reasonable default or would it be passed with Basis? It causes issue if we are trying to keep track of what the user passes though for building the model
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 do not have a strong opinion so I suggest whatever you feel is easiest to maintain. We can release and collect feedback.
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.
Looks very nice! I left som minor comments.
Are the open to-dos meant to be included in this PR?
def create_basis_matrix(s_ref, e_ref): | ||
return pt.where( | ||
(s_ref >= 0) & (e_ref <= 0), | ||
0, | ||
pt.where(pt.abs(s_ref) < pt.abs(e_ref), s_ref, e_ref), |
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.
seems reasonable
Addressed the feedback. I will make the todos into new issues |
Working on the out of sample now |
This should be good to go now. I will work toward the storing off of the events in another PR (#1484) |
This is very cool! I'll try to see if I can add this to the mmm example notebook as well have two events. |
Description
The API
TODO:
Related Issue
Checklist
pre-commit.ci autofix
to auto-fix.