-
Notifications
You must be signed in to change notification settings - Fork 16
feat: Pivot table example generator #1230
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
Conversation
|
plotly-express docs preview (Available for 14 days) |
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Outdated
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Outdated
Show resolved
Hide resolved
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
| Returns a fish market sales dataset designed for pivot table examples. Ticks every second, | ||
| is random but deterministic, and contains lots of categorical data for pivoting. | ||
| Columns: |
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 really like the column breakdown, that's something that we should add to all of the example datasets as really what is important is the columns in the table, not the arguments. Probably beyond the scope of this PR though.
Going further, and absolutely beyond the scope of this, I wonder if we could develop some sort of standardization with "column descriptions" for functions that return tables (and maybe take them if certain columns are expected?). Something might already exist for this but I couldn't find anything quickly. I think it's a missed opportunity thought because then we could build tooling around it for docs, tooltips, etc.
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.
there was a concept of table column descriptions, that would show up in the tooltips, but I don't think it was ever fully built out. Added docs for all the tables.
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 JS API has column descriptions wired up, and the UI displays the description in the tooltip when you hover over the header.
Only time that description actually gets used right now I think is if it's a preview column (the description will say something like "Preview of ...").
Not wired up in Column on the engine side though.
|
plotly-express docs preview (Available for 14 days) |
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.
Pull Request Overview
This PR adds a new pivot table example generator called fish_market and makes performance improvements to the stocks data generator. It also standardizes documentation formatting across data generators.
- Adds
fish_market()generator with comprehensive categorical data suitable for pivot table examples - Replaces stocks table replayer with
merge(empty_table, time_table)approach for better performance - Standardizes documentation format across all data generators with consistent column descriptions
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
data_generators.py |
Adds fish_market generator, optimizes stocks performance, fixes type conversions, standardizes docstrings |
__init__.py |
Exports new fish_market function |
| Documentation snapshots | Snapshot files for all data generators in documentation |
sidebar.json |
Adds example data documentation to sidebar |
example-data.md |
New documentation page describing all available example datasets |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Show resolved
Hide resolved
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
plugins/plotly-express/src/deephaven/plot/express/data/data_generators.py
Outdated
Show resolved
Hide resolved
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.
What is happening here? Unstable test?
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.
That shouldn't have been in this pr, it looks like a timeout result, but also weird that it passed in CI.
|
plotly-express docs preview (Available for 14 days) |
Note: Because of how random was used stocks random string will change (triggering e2e test), and ticking false has changed from 36,000 rows to a more reasonable 3,000 rows.