feat: Add aggregate parameters to vega-transform, and exponential moving average #3686

Xitian9 · 2023-03-12T11:06:33Z

This adds exponential moving averages, fixing #3341. In order to do this, it enables supplying parameters to aggregation functions. This addresses the block on vega/vega-lite#4439.

As I'm not a javascript programmer, feedback will be gratefully taken on board.

…ethods

Xitian9 · 2023-03-24T02:52:44Z

Is there any interest in this?

domoritz · 2023-03-24T03:29:01Z

Thanks for the pull request. I just approved the test run again.

Xitian9 · 2023-04-09T09:16:18Z

If/when this is merged, #3577 could be made more general by passing the requested quantiles as a parameter.

shcheklein · 2023-05-11T23:41:07Z

Hey @Xitian9 thanks for the effort! ~~This is exactly what we are looking for~~ It might be what we are looking for in this case iterative/vscode-dvc#3837 . In ML tracker tools it's very common to use the exponential moving average. I'm still looking into this (and learning along the way if there are other ways to achieve this).

Can it be used to smooth a linear plot or does it calculate a single number?

@domoritz are there any blockers left to merge this? (maybe we can also join and help). Also, if this is merged will it become available automatically in vega-lite?

domoritz

Looks reasonable to me. The API question I have is whether it makes sense for the parameter to be a separate property or somehow part of the ops. I guess this is consistent with the rest of the API.

@jheer needs to make a final pass and merge

domoritz · 2023-05-12T19:17:12Z

packages/vega-transforms/src/Aggregate.js

@@ -11,6 +11,7 @@ import {accessorFields, accessorName, array, error, inherits} from 'vega-util';
 * @param {Array<function(object): *>} [params.groupby] - An array of accessors to groupby.
 * @param {Array<function(object): *>} [params.fields] - An array of accessors to aggregate.
 * @param {Array<string>} [params.ops] - An array of strings indicating aggregation operations.
+ * @param {Array<object>} [params.aggregate_params=[null]] - An optional array of parameters for aggregation operations.


Do we need to repeat the name aggregate here or is it already obvious because of the context of the aggregate?

I was going by the naming agreed upon in vega/vega-lite#4439. We cannot just call it params.params, as this would conflict with the params argument in windowing functions. Anytime you would want to apply an aggregate with parameter over a window with parameter, you would run into problems.

Xitian9 · 2023-05-14T11:36:55Z

Can it be used to smooth a linear plot or does it calculate a single number?

The aggregate in itself produces a single number (which is why I call it exponential instead of exponentialmovingaverage or something like that). However, it can be combined with a windowing function to produce a smoothing, which is the use case I have in mind as well.

@domoritz are there any blockers left to merge this? (maybe we can also join and help). Also, if this is merged will it become available automatically in vega-lite?

I don't believe it would automatically be available in vega-lite, and that would need to be updated as well.

shcheklein · 2023-06-19T19:41:30Z

@jheer sorry for bothering you, anything we can do (help with) here to unblock this and move forward? Please let me know.

Xitian9 · 2023-08-08T10:12:10Z

Is there any appetite for this? If not I'll delete the PR.

shcheklein · 2023-08-08T19:09:27Z

@Xitian9 please keep it for a while :) We definitely need this! I can't make decisions here and / or review, but please keep it possible. I hope folks will get some time to merge it. Thanks for your work.

shcheklein · 2023-09-19T22:18:50Z

Hey folks, @jheer , any chance we can move it forward? Could we at least identify what is blocking this PR atm?

jheer · 2023-09-19T22:51:13Z

Thanks for the PR, sorry for the delay in reviewing!

I checked with @jonmmease and expanding aggregates this way does not have negative downstream consequences for VegaFusion (other than that weighted exponential aggregates will not be optimized by VegaFusion at the moment).

julieg18 · 2023-12-14T13:43:56Z

Hello, I'm working on adding aggregate_parameters into vega-lite (vega/vega-lite#9200), but when I tested using the params in a vega editor spec, all I could get was NaN:

I tried a couple different field variations but NaN was always the result. Have I misunderstood what aggregate_params is supposed to consist of? Here's the example vega editor spec I was using.

domoritz · 2023-12-14T14:36:03Z

packages/vega-transforms/src/Aggregate.js

@@ -45,6 +46,7 @@ Aggregate.Definition = {
  'params': [
    { 'name': 'groupby', 'type': 'field', 'array': true },
    { 'name': 'ops', 'type': 'enum', 'array': true, 'values': ValidAggregateOps },
+    { 'name': 'aggregate_params', 'type': 'field', 'null': true, 'array': true, 'default': [null] },


This doesn't look right. In the docs, we say object and then here the type is field.

domoritz · 2023-12-14T14:48:20Z

@julieg18 hmm, yeah, something is wrong here. The types look wrong to me.

I think the aggregate parameter is just a value, not a field judging from the code but maybe I'm missing something: Open the Chart in the Vega Editor (still fails because of some error somewhere).

@Xitian9 @shcheklein can you post an example you used to test this feature? Also, could you send a follow up pull request to add docs.

domoritz · 2023-12-14T14:50:27Z

packages/vega-transforms/src/Window.js

@@ -37,6 +38,7 @@ Window.Definition = {
    { 'name': 'groupby', 'type': 'field', 'array': true },
    { 'name': 'ops', 'type': 'enum', 'array': true, 'values': ValidWindowOps.concat(ValidAggregateOps) },
    { 'name': 'params', 'type': 'number', 'null': true, 'array': true },
+    { 'name': 'aggregate_params', 'type': 'field', 'null': true, 'array': true, 'default': [null] },


Is the default correct here? There may be more than two ops.

The code will pad aggregate_params with nulls as necessary to match the number of ops.

domoritz · 2023-12-14T15:01:19Z

packages/vega-transforms/src/Aggregate.js

@@ -11,6 +11,7 @@ import {accessorFields, accessorName, array, error, inherits} from 'vega-util';
 * @param {Array<function(object): *>} [params.groupby] - An array of accessors to groupby.
 * @param {Array<function(object): *>} [params.fields] - An array of accessors to aggregate.
 * @param {Array<string>} [params.ops] - An array of strings indicating aggregation operations.
+ * @param {Array<object>} [params.aggregate_params=[null]] - An optional array of parameters for aggregation operations.


Shouldn't this be [params.aggregate_params] since we cannot assume a single op?

The code will pad aggregate_params with nulls as necessary to match the number of ops.

Xitian9 · 2023-12-14T23:39:15Z

@Xitian9 @shcheklein can you post an example you used to test this feature? Also, could you send a follow up pull request to add docs.

Test code is here: https://github.com/vega/vega/blob/main/packages/vega-transforms/test/aggregate-test.js

Xitian9 · 2023-12-14T23:42:33Z

Hello, I'm working on adding aggregate_parameters into vega-lite (vega/vega-lite#9200), but when I tested using the params in a vega editor spec, all I could get was NaN:

I tried a couple different field variations but NaN was always the result. Have I misunderstood what aggregate_params is supposed to consist of? Here's the example vega editor spec I was using.

"exponential" takes a number as an argument, and you have provided the string "param_value". Do you still get NaN if you supply a double?

Xitian9 · 2023-12-14T23:45:07Z

@julieg18 hmm, yeah, something is wrong here. The types look wrong to me.

I think the aggregate parameter is just a value, not a field judging from the code but maybe I'm missing something:

I'm not a javascript programmer, so I could definitely believe I got the types wrong. The type needs to be generic enough to accept any sort of value, as different ops will have different argument types. For "exponential" that's a number, but for other as-yet-unimplemented ops it could be something of arbitrary complexity.

julieg18 · 2023-12-15T17:46:36Z

"exponential" takes a number as an argument, and you have provided the string "param_value".

Makes sense, thanks! As previously mentioned, I can only provide strings since currently the type is set to fields.

Do you still get NaN if you supply a double?

Apologies, I'm not sure what you mean by double? Do you mean something like this:

I would still get NaN in this case

domoritz · 2023-12-15T19:27:39Z

Makes sense, thanks! As previously mentioned, I can only provide strings since currently the type is set to fields.

The editor will only warn you. Numbers are still passed to Vega.

julieg18 · 2023-12-15T23:21:29Z

The editor will only warn you. Numbers are still passed to Vega.

~~Perhaps I'm misunderstanding something, but inputting numbers causes errors to be thrown and nothing get rendered, in the view or data viewer~~ Oh wait, I think I understand what you're saying. You're saying that that those numbers are actually getting passed to vega. These errors aren't just being thrown because of the wrong type, there is something else happening.

julieg18 · 2024-01-05T00:33:04Z

Types are now fixed thanks to #3846, but as expected, we are still hitting the Unexpected token ']' error when trying to run a spec.

Looking at the error locally, it's possibly happening inside of vega-runtime:

VM823:3 Uncaught SyntaxError: Unexpected token ']'
    at Function (<anonymous>)
    at get (vega-runtime.module.js:142:19)
    at field (vega-util.module.js:93:38)
    at Object.getField2 [as parse] (vega-runtime.module.js:281:36)
    at parseParameter (vega-runtime.module.js:212:16)
    at vega-runtime.module.js:199:51
    at Array.map (<anonymous>)
    at Context.parseParameters (vega-runtime.module.js:199:42)
    at Context.parseOperatorParameters (vega-runtime.module.js:187:48)
    at vega-runtime.module.js:30:34

@Xitian9, are you able to get a working example of aggregate_params in the vega editor?

julieg18 · 2024-01-10T13:29:08Z

Types are now fixed thanks to #3846, but as expected, we are still hitting the Unexpected token ']' error when trying to run a spec.

Actually, I tested aggregate_params today with the latest version of vega in my local vega editor and everything seems to be working!

Not sure why I was getting the error previously, but unless it shows up again, I'll get back to vega/vega-lite#9200

domoritz · 2024-01-10T13:56:42Z

Awesome. Thanks for the update. I'm glad it's working now.

Xitian9 force-pushed the aggparam branch from 446bbc4 to 6035cde Compare March 12, 2023 20:48

Xitian9 added 2 commits March 13, 2023 09:01

feat(vega-transforms): enable parameters for aggregation operations

5b07d81

feat(vega-transforms): add exponential and exponentialb aggregation m…

4445688

…ethods

Xitian9 force-pushed the aggparam branch from 6035cde to 4445688 Compare March 12, 2023 22:02

shcheklein mentioned this pull request May 11, 2023

Incorrect display smooth feature iterative/vscode-dvc#3837

Open

domoritz approved these changes May 12, 2023

View reviewed changes

domoritz requested a review from jheer May 12, 2023 19:19

jheer merged commit 231a918 into vega:main Sep 19, 2023
3 checks passed

Xitian9 mentioned this pull request Nov 9, 2023

feat: Expose aggregate parameters through vega-typings #3827

Merged

julieg18 mentioned this pull request Dec 12, 2023

Add exponential moving average to vega-lite vega/vega-lite#9200

Open

1 task

domoritz reviewed Dec 14, 2023

View reviewed changes

domoritz mentioned this pull request Dec 14, 2023

fix: correct aggregate params types #3846

Merged

julieg18 mentioned this pull request Feb 13, 2024

fix: add missing aggregate_params to window transform type #3874

Merged

domoritz mentioned this pull request Feb 18, 2024

Should aggregate_params be aggregateParams #3876

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add aggregate parameters to vega-transform, and exponential moving average #3686

feat: Add aggregate parameters to vega-transform, and exponential moving average #3686

Xitian9 commented Mar 12, 2023

Xitian9 commented Mar 24, 2023

domoritz commented Mar 24, 2023

Xitian9 commented Apr 9, 2023

shcheklein commented May 11, 2023 •

edited

domoritz left a comment

domoritz May 12, 2023

Xitian9 May 14, 2023 •

edited

Xitian9 commented May 14, 2023 •

edited

shcheklein commented Jun 19, 2023

Xitian9 commented Aug 8, 2023

shcheklein commented Aug 8, 2023

shcheklein commented Sep 19, 2023

jheer commented Sep 19, 2023

julieg18 commented Dec 14, 2023

domoritz Dec 14, 2023

domoritz Dec 14, 2023

domoritz commented Dec 14, 2023 •

edited

domoritz Dec 14, 2023

Xitian9 Dec 14, 2023

domoritz Dec 14, 2023

Xitian9 Dec 14, 2023

Xitian9 commented Dec 14, 2023

Xitian9 commented Dec 14, 2023

Xitian9 commented Dec 14, 2023

julieg18 commented Dec 15, 2023

domoritz commented Dec 15, 2023

julieg18 commented Dec 15, 2023 •

edited

julieg18 commented Jan 5, 2024

julieg18 commented Jan 10, 2024

domoritz commented Jan 10, 2024

feat: Add aggregate parameters to vega-transform, and exponential moving average #3686

feat: Add aggregate parameters to vega-transform, and exponential moving average #3686

Conversation

Xitian9 commented Mar 12, 2023

Xitian9 commented Mar 24, 2023

domoritz commented Mar 24, 2023

Xitian9 commented Apr 9, 2023

shcheklein commented May 11, 2023 • edited

domoritz left a comment

Choose a reason for hiding this comment

domoritz May 12, 2023

Choose a reason for hiding this comment

Xitian9 May 14, 2023 • edited

Choose a reason for hiding this comment

Xitian9 commented May 14, 2023 • edited

shcheklein commented Jun 19, 2023

Xitian9 commented Aug 8, 2023

shcheklein commented Aug 8, 2023

shcheklein commented Sep 19, 2023

jheer commented Sep 19, 2023

julieg18 commented Dec 14, 2023

domoritz Dec 14, 2023

Choose a reason for hiding this comment

domoritz Dec 14, 2023

Choose a reason for hiding this comment

domoritz commented Dec 14, 2023 • edited

domoritz Dec 14, 2023

Choose a reason for hiding this comment

Xitian9 Dec 14, 2023

Choose a reason for hiding this comment

domoritz Dec 14, 2023

Choose a reason for hiding this comment

Xitian9 Dec 14, 2023

Choose a reason for hiding this comment

Xitian9 commented Dec 14, 2023

Xitian9 commented Dec 14, 2023

Xitian9 commented Dec 14, 2023

julieg18 commented Dec 15, 2023

domoritz commented Dec 15, 2023

julieg18 commented Dec 15, 2023 • edited

julieg18 commented Jan 5, 2024

julieg18 commented Jan 10, 2024

domoritz commented Jan 10, 2024

shcheklein commented May 11, 2023 •

edited

Xitian9 May 14, 2023 •

edited

Xitian9 commented May 14, 2023 •

edited

domoritz commented Dec 14, 2023 •

edited

julieg18 commented Dec 15, 2023 •

edited