-
Notifications
You must be signed in to change notification settings - Fork 235
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
#990 - Plot FourierBase along date rather than index #1068
#990 - Plot FourierBase along date rather than index #1068
Conversation
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.
Thanks for taking this @Ishaanjolly !
Some initial comments and suggestions
Could you write a test for the sample plot workflow using dates
pymc_marketing/mmm/fourier.py
Outdated
if isinstance(self, YearlyFourier): | ||
start_date = datetime.datetime(year=today.year, month=1, day=1) | ||
elif isinstance(self, MonthlyFourier): | ||
start_date = datetime.datetime( | ||
year=today.year, month=today.month, day=1 | ||
) |
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.
Is it possible to not rely on the child classes? This makes an inheritance from the FourierBase
not possible.
This can usually be solved by a method in each child class. For instance, get_default_start_date(today: datetime)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1068 +/- ##
==========================================
- Coverage 95.85% 95.23% -0.62%
==========================================
Files 39 39
Lines 3934 3969 +35
==========================================
+ Hits 3771 3780 +9
- Misses 163 189 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…te and reverted to using isinstance |
if start_date is None: | ||
return self._get_default_start_date() | ||
elif isinstance(start_date, str) | isinstance(start_date, datetime.datetime): | ||
return start_date |
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.
Subtlety here is that user can pass date that doesn't start at beginning of month or year and the plot starts.
I think that's fine if someone adjusts the zero point in x / dayofyear
. Though conventionally that'd be beginning of month or year.
Just thinking out loud
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 good! Thanks @Ishaanjolly
Description
I added the following:
and following within each plot_* function as I was not able to define new attributes:
Related Issue
FourierBase
along date rather than index #990Checklist
Modules affected
Type of change
📚 Documentation preview 📚: https://pymc-marketing--1068.org.readthedocs.build/en/1068/