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 joinaggregate. Fixes #4121 #4554
Conversation
e075987
to
8545768
Compare
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.
-
Let's keep only one window example that is equivalent to joinaggregate to avoid redundancy
-
Update the advanced calculation to show joinaggregate-based example when window is not necessary
@@ -0,0 +1 @@ | |||
examples/specs/*.vl.json |
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.
Can we put this rule in .prettierrc.json
?
If we can, probably better to consolidate them in one place.
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 don't think it works yet. prettier/prettier#3460
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.
Don't replace window
with joinaggregate
if the frame is not [null, null]
.
The results will be incorrect.
@@ -12,13 +12,10 @@ | |||
"format": {"type": "json"}, | |||
"transform": [ | |||
{ | |||
"type": "window", | |||
"params": [null], | |||
"type": "joinaggregate", |
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.
Cumulative frequency should not use join aggregate.
Window with frame = [null, 0]
makes more sense. (See SVG)
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.
Ups, my mistake.
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.
The code should only do it when the frame is [null, null]
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.
This was a bug in the code. Fixed.
82641d6
to
7b7d4c6
Compare
Instead of just doing it in the code, I think better update all the examples (except one of them) to use joinaggregate in VL examples too. |
That's what I did. Which ones did I miss? |
Fixed bar_diverging as well |
@kanitw This is ready |
No description provided.