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.

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

Transform inverse mapping #2126

Merged
merged 8 commits into from
Oct 27, 2017
Merged

Transform inverse mapping #2126

merged 8 commits into from
Oct 27, 2017

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Oct 26, 2017

  • generating original points for transform results so that, upon interaction such as crossfiltering, the original (constituent) points can be recovered
  • cascading these points through a (purportedly) arbitrary sequence of filter and aggregation steps (groupby not yet supported)
  • adding test cases for these (also, we didn't have heterogeneous transforms testing with both filter and aggregate in the past)

There's still so much to do (PS: now documented the follow-up issue #2128); eg. more test cases; covering groupBy and sort. There's plenty of things to iterate on here.

On the pro side, the code change is rather small in terms of lines; also, the logic didn't need to change. So it's an add-on patch rather than anything heavy.

@monfera monfera self-assigned this Oct 26, 2017
}

opts.indexToPoints = indexToPoints;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this isn't a real attribute but we're putting it back into fullData lets give it an _ - ie opts._indexToPoints.


done();
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing this! Nice to see indexToPoints cascade through the transforms.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_ is the only blocking comment, this looks great. Really clean and easy!

With this in place you can get what you need for crossfiltering, and nothing you did will need to change as we extend this, so I'm all for merging this with only that change.

But to use this mapping, you have to dive into _fullData and look for transforms. Seems like it would be more powerful if this were already part of the event data. I'm not quite sure the right way to do this though... possibly just attach a new attribute like event.points[i].inputPointNumbers? If there are no transforms this would just be [pointNumber] or pointNumbers, but if there are, it would map whichever of those exists through _indexToPoints? That way you could always just use inputPointNumbers. Also since any trace type could have a transform (and at this point all or nearly all types produce pointNumber(s) fields), it would be nice to find a clean way to apply this to all events consistently. But again, lets do that in a separate PR. Really great idea to go in this direction, and I'm pleased that it's turning out to be pretty straightforward to implement!

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thanks! 💃

@monfera monfera merged commit fd3a5b8 into master Oct 27, 2017
@monfera monfera deleted the transform-inverse-mapping branch October 27, 2017 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants