-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Feature: quarterly interval in cohort #4269 #5342
base: master
Are you sure you want to change the base?
Conversation
I'd never changed the source structure, only added some 'interval' related options like "quarterly" etc. How could I pass the build and e2e-tests? |
You passed! 😅 |
Hi @zjsun thanks for your work here. I took a look at the implementation of this feature and despite the categories showing up properly in the UI, the logic behind them seems to not work properly. That is, the quarters do not match with the monthly results. Please tell me if otherwise. It is basically defined here (line 40 of _.each(grouped, group => {
if (previousDate !== null) {
// @ts-expect-error ts-migrate(2571) FIXME: Object is of type 'unknown'.
let diff = Math.abs(previousDate.diff(group.date, momentInterval[timeInterval]));
while (diff > 1) {
const row = [0];
for (let stage = firstStage; stage <= lastStage; stage += 1) {
// @ts-expect-error ts-migrate(2571) FIXME: Object is of type 'unknown'.
row.push(group.values[stage] || 0);
}
data.push(row);
// It should be diagonal, so decrease count of stages for each next row
lastStage -= 1;
diff -= 1;
}
} I couldn't grasp immediately why it didn't work, so I can better investigate it soon, if you want to. |
Thanks for your reviewing. @rafawendel When using quarterly interval option, The cohort date in the query result is based on the rule which is based on the first day of the quarter(which is mentioned here ), eg. 2019-01-01, 2019-04-01, 2019-07-01 ... etc. FYI. Here is my query result: |
It is already difficult to merge the code here. You have another option available: |
@zjsun , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI? We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that. |
Codecov Report
@@ Coverage Diff @@
## master #5342 +/- ##
=======================================
Coverage 60.70% 60.70%
=======================================
Files 154 154
Lines 12624 12624
Branches 1716 1716
=======================================
Hits 7663 7663
Misses 4735 4735
Partials 226 226 |
What type of PR is this? (check all applicable)
Description
Support quarterly interval in cohort #4269
Related Tickets & Documents
#4269
Mobile & Desktop Screenshots/Recordings (if there are UI changes)