Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Plot FourierBase along date rather than index #990

Closed
wd60622 opened this issue Aug 29, 2024 · 3 comments · Fixed by #1068
Closed

Plot FourierBase along date rather than index #990

wd60622 opened this issue Aug 29, 2024 · 3 comments · Fixed by #1068
Labels
enhancement New feature or request good first issue Good for newcomers . Doesn't require extensive knowledge of the repo and package plots

Comments

@wd60622
Copy link
Contributor

wd60622 commented Aug 29, 2024 •
edited
Loading

The plotting of the fourier is done along the index rather than a date which can be confusing to the meaning of the x-axis. The can be changed by allowing the sample_curve function to set the coords to datetimes instead of integers.

What date would be the start of the fourier curve though? Might want to derive from the current year or current month.

Overall, this would be optional and be an alternative to the current functionality.
@wd60622 wd60622 added enhancement New feature or request plots good first issue Good for newcomers . Doesn't require extensive knowledge of the repo and package labels Aug 29, 2024
@wd60622 wd60622 assigned wd60622 and unassigned wd60622 Sep 8, 2024
@Ishaanjolly
Copy link
Contributor

Would you suggest the user is able to specify the start_date and if not it is derived from the current year /month?

@wd60622
Copy link
Contributor Author

wd60622 commented Sep 24, 2024

Yes, I think it would be good to allow user to specify the start date if they want to. Deriving from the current year/month seems like a good default. The fourier period will always be the same regardless of the month though, it really just comes down to the labels.

@Ishaanjolly Ishaanjolly mentioned this issue Sep 25, 2024
Merged
13 tasks
@Ishaanjolly
Copy link
Contributor

@wd60622 - I have addressed this enhancement in my latest PR - please feel free to give feedback!

juanitorduz pushed a commit that referenced this issue Oct 2, 2024
@Ishaanjolly @wd60622
#990 - Plot FourierBase along date rather than index (#1068)
2187dd3 
* feat: test.txt added for commit check

* feat: replaced plot_curve with plot_samples within ./mmm/plot.py

* feat: n_samples added to distributions_new_customers

* remove: text.txt from initial commit

* feat(fourier.py): added the custom plotting

* fix(full_period): so that there is 367 instead of 366 as pointed out in CI

* added two new methods to the class - get_default_start_dates and _get_default_start_dates

* feat(fourier.py): modified full_period

* fix(fourier.py): removes datetime conversions where not req

* feat(test_fourier.py): added new tests for the date addition to fourier.py workflow

* : fix: removed today from all parameters

* feat(test_fourier.py): new tests after removing today from params in fourier.py

* fix(test_fourier.py): removed days_in_period

* fix(fourier.py): removed the comment with <3.10

* fix(mmm/fourier.py): changed else: if to elif in get_default_start_date and reverted to using isinstance |

---------

Co-authored-by: Will Dean <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request good first issue Good for newcomers . Doesn't require extensive knowledge of the repo and package plots
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

#990 - Plot FourierBase along date rather than index Ishaanjolly/pymc-marketing
2 participants
@wd60622 @Ishaanjolly