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 Peak Month
aggregation function.
#3006
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.
Big picture is the same as my previous review. I think the calculations should be less discrete, and open the potential for figuring out the peak day in a year, or peak month, or peak week (Germans would like this).
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.
Front end changes look good.
All my concerns from Peak Day of Week apply here as well though.
Co-authored-by: Brent Moran <brentmoran@gmail.com>
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 great overall! Thanks for the great comments explaining the functions.
The only change I have to request is to remove the casting of output months to strings. They should either be integers, or 2-digit formatted numeric strings (e.g., '01', '02', '11', '12'
)
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.
Okay, this looks good. Thank you!
Fixes #3005
This PR adds
Peak Month
aggregation function to the Mathesar UI.Technical details
I have done the following things:
Peak Month
aggregation.Peak Month
to the Mathesar UI.Screenshots
Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin