-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Plotly express downsampling #453
Merged
mattrunyon
merged 12 commits into
deephaven:main
from
mattrunyon:plotly-express-downsampling
May 15, 2024
Merged
feat: Plotly express downsampling #453
mattrunyon
merged 12 commits into
deephaven:main
from
mattrunyon:plotly-express-downsampling
May 15, 2024
+6,068
−6,578
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattrunyon
force-pushed
the
plotly-express-downsampling
branch
from
May 8, 2024 23:05
7b5b294
to
b8b7371
Compare
jnumainville
added a commit
that referenced
this pull request
May 10, 2024
Tests were failing on #453 due to some parsing issues This is similar to deephaven/deephaven-core#5373 which I thought was fixed. Regardless, we shouldn't be using `to_j_instant` in this way due to poor performance. For now I think assuming the column is an `Instant` is fine.
mattrunyon
force-pushed
the
plotly-express-downsampling
branch
from
May 10, 2024 16:30
a2a4210
to
f730c08
Compare
mofojed
previously approved these changes
May 10, 2024
mofojed
requested changes
May 13, 2024
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.
Just clean up some of the docs for the magic values.
plugins/plotly-express/src/js/src/PlotlyExpressChartModel.test.ts
Outdated
Show resolved
Hide resolved
mofojed
requested changes
May 14, 2024
plugins/plotly-express/src/js/src/PlotlyExpressChartModel.test.ts
Outdated
Show resolved
Hide resolved
mofojed
approved these changes
May 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #41
Here's a test that you can see the re-downsample is triggered when zooming in. Also that it still ticks properly and handles multiple series (the Python side creates 2 tables in this case)
Plot by example
This code shows layering which will use the same table. This only works if you have a shared X column because of how downsampling works. The alternative is to create copies of the table and downsample each copy separately, but I figured we can probably leave that alone unless somebody requests it since it seems like a pretty rare case.