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.

Sign up for GitHub

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

Jump to bottom

Scatter chart #7164

Merged
merged 67 commits into from Sep 1, 2023
Merged

Scatter chart #7164

merged 67 commits into from Sep 1, 2023

Conversation

sfc-gh-tteixeira
Copy link
Contributor

Describe your changes

Add support for st.scatter_chart!

This is PR 4 in the work to:

  • Add latitude, longitude, color, and size to to st.map. This was PR 2.
  • Add a color argument to our native charts. This was PR 3
  • Add st.scatter_chart. 👈👈👈 THAT'S THIS PR!

(PR 1 was a no-op that added the color_util.py library, which is used in this and the next PRs)

GitHub Issue Link (if applicable)

Testing Plan

  • N/A - Explanation of why no additional tests are needed

  • ✅ Unit Tests (JS and/or Python)

  • ✅ E2E Tests

  • Any manual testing needed?

    Yes. I'll request this from the QA team soon.

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@sfc-gh-tteixeira
Add color support to charts, and refactor things a bit.
de37f07
@sfc-gh-tteixeira
Fix tests and types.
3ac3e4f
@sfc-gh-tteixeira
Fix types in type_util
c2e4dcc
@sfc-gh-tteixeira
Removin unnecessary DF cloning, and add test to ensure we're not muta…
58a082b 
…ting the orig DF.
@sfc-gh-tteixeira
Fix tests in latest Pandas and Arrow.
071b37d
@sfc-gh-tteixeira
Revert st_arrow_add_rows test to avoid too many changes in this PR. W…
b7d2bf3 
…ill land in another PR.
@sfc-gh-tteixeira
Remove snaps that should be regenerated.
71df8c6
@sfc-gh-tteixeira
Merge remote-tracking branch 'origin/develop' into chart_colors
e660fa6
@sfc-gh-tteixeira
Don't show labels when no x or y specified (to match existing behavior).
9dfd7b7
@sfc-gh-tteixeira
Update snapshots (need to regenerate)
61c8ea8
@sfc-gh-tteixeira
Fix bug where x axis was sometimes quantitative rather than temporal.
3f1a06a
@sfc-gh-tteixeira
Implement st.scatter_chart
909fc62
@sfc-gh-tteixeira
Fix x enc when no x_column.
7ddbe74
@sfc-gh-tteixeira
Merge branch 'chart_colors' into scatter_chart
fcbfe21
@sfc-gh-tteixeira
Add more scatter tests
c47bd58
@sfc-gh-tteixeira
Fix tests.
5bc4ab9
@sfc-gh-tteixeira
Revert to melting data by hand since Snowflake doesn't include Altair…
6f2f460 
… 4.2
@sfc-gh-tteixeira
Merge branch 'chart_colors' into scatter_chart
cf76214
@sfc-gh-tteixeira
Improve error messages.
711cc0a
@sfc-gh-tteixeira
Merge branch 'chart_colors' into scatter_chart
66b250b
@sfc-gh-tteixeira
Fix MyPy types
43e2b0e
@sfc-gh-tteixeira
Merge branch 'chart_colors' into scatter_chart
f5efbda
@sfc-gh-tteixeira
Update snapshots
60c4bd0
@sfc-gh-tteixeira
Fix empty check
10a2a4a
@sfc-gh-tteixeira
Merge branch 'chart_colors' into scatter_chart
5bc025f
@sfc-gh-tteixeira
Revert setup.py
08c9750
@sfc-gh-tteixeira
Remove dead code.
416b054
@sfc-gh-tteixeira
simple rename
03d1ffd
@sfc-gh-tteixeira
Remove more dead code
51b29ed
@sfc-gh-tteixeira
Merge branch 'chart_colors' into scatter_chart
6882104
willhuang1997
willhuang1997 reviewed Aug 18, 2023
View reviewed changes
lib/streamlit/elements/arrow_altair.py
# through trial-and-error I found this value to be a good enough middle ground.
# See e2e/scripts/st_arrow_scatter_chart.py for some alignment tests.
COLOR_LEGEND_SETTINGS = dict(titlePadding=5, offset=5, orient="bottom")
SIZE_LEGEND_SETTINGS = dict(titlePadding=0.5, offset=5, orient="bottom")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved all I could to CustomTheme.tsx, but I need to leave the titlePadding here in Python because there's no way to set a different value for one encoding (i.e. color) vs another encoding (size).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I just realized that by moving this we broke some existing behavior.

When you create an Altair/Vega chart by hand, we used to (1) apply Streamlit colors, but (2) NOT move the legend to the bottom. With this change, we now DO move the legend to the bottom.

I need to double-check whether #2 was the desired behavior or just a bug.

willhuang1997
willhuang1997 approved these changes Aug 18, 2023
View reviewed changes
Copy link
Collaborator

@willhuang1997 willhuang1997 left a 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 there's anything glaring about this PR so I'm going to give approval. However, please fix all of the comments please :)

@sfc-gh-tteixeira
Copy link
Contributor Author

QA request filed!

@mayagbarnes
Copy link
Collaborator

FWIW re: Python test failures:
test_arrow_chart_with_explicit_x_plus_y_and_color_sequence checks here for the legend specification set with (now removed) COLOR_LEGEND_SETTINGS in _get_color_encoding.

@kasim-inan kasim-inan added the QA-Done QA is Complete label Sep 1, 2023
@sfc-gh-tteixeira sfc-gh-tteixeira merged commit bdd1d9b into streamlit:develop Sep 1, 2023
52 checks passed
@sfc-gh-tteixeira sfc-gh-tteixeira deleted the scatter_chart branch September 1, 2023 21:18
eric-skydio pushed a commit to eric-skydio/streamlit that referenced this pull request Dec 20, 2023
@sfc-gh-tteixeira @eric-skydio
Scatter chart (streamlit#7164)
6c46c9a 
* Add color support to charts, and refactor things a bit.

* Fix tests and types.

* Fix types in type_util

* Removin unnecessary DF cloning, and add test to ensure we're not mutating the orig DF.

* Fix tests in latest Pandas and Arrow.

* Revert st_arrow_add_rows test to avoid too many changes in this PR. Will land in another PR.

* Remove snaps that should be regenerated.

* Don't show labels when no x or y specified (to match existing behavior).

* Update snapshots (need to regenerate)

* Fix bug where x axis was sometimes quantitative rather than temporal.

* Implement st.scatter_chart

* Fix x enc when no x_column.

* Add more scatter tests

* Fix tests.

* Revert to melting data by hand since Snowflake doesn't include Altair 4.2

* Improve error messages.

* Fix MyPy types

* Update snapshots

* Fix empty check

* Revert setup.py

* Remove dead code.

* simple rename

* Remove more dead code

* Address comments.

* Address comments.

* Updating screenshot

* Fix snapshots

* Fix stupid copy/paste bug >_<

* Fix stupid copy/paste bug >_<

* Fix merge issues

* Add snapshots

* Small cleanup. Always use str() around dataset ids. Should be a no-op since JSON stringifies keys anyways, but this is cleaner.

* Fix doc bug.

* REALLY fix the doc bug

* Improve docstrings

* Address comments. Move legend settings to CustomTheme.tsx

* Rename test images.

* Address comments. Fixed test.

* Undo move of legend config to CustomTheme.tsx as it broke existing behavior.

* Fix tests

* Fix legend test.
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Mar 22, 2024
@sfc-gh-tteixeira
Scatter chart (streamlit#7164)
8f9f78d 
* Add color support to charts, and refactor things a bit.

* Fix tests and types.

* Fix types in type_util

* Removin unnecessary DF cloning, and add test to ensure we're not mutating the orig DF.

* Fix tests in latest Pandas and Arrow.

* Revert st_arrow_add_rows test to avoid too many changes in this PR. Will land in another PR.

* Remove snaps that should be regenerated.

* Don't show labels when no x or y specified (to match existing behavior).

* Update snapshots (need to regenerate)

* Fix bug where x axis was sometimes quantitative rather than temporal.

* Implement st.scatter_chart

* Fix x enc when no x_column.

* Add more scatter tests

* Fix tests.

* Revert to melting data by hand since Snowflake doesn't include Altair 4.2

* Improve error messages.

* Fix MyPy types

* Update snapshots

* Fix empty check

* Revert setup.py

* Remove dead code.

* simple rename

* Remove more dead code

* Address comments.

* Address comments.

* Updating screenshot

* Fix snapshots

* Fix stupid copy/paste bug >_<

* Fix stupid copy/paste bug >_<

* Fix merge issues

* Add snapshots

* Small cleanup. Always use str() around dataset ids. Should be a no-op since JSON stringifies keys anyways, but this is cleaner.

* Fix doc bug.

* REALLY fix the doc bug

* Improve docstrings

* Address comments. Move legend settings to CustomTheme.tsx

* Rename test images.

* Address comments. Fixed test.

* Undo move of legend config to CustomTheme.tsx as it broke existing behavior.

* Fix tests

* Fix legend test.
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
@sfc-gh-tteixeira @zyxue
Scatter chart (streamlit#7164)
d7a1428 
* Add color support to charts, and refactor things a bit.

* Fix tests and types.

* Fix types in type_util

* Removin unnecessary DF cloning, and add test to ensure we're not mutating the orig DF.

* Fix tests in latest Pandas and Arrow.

* Revert st_arrow_add_rows test to avoid too many changes in this PR. Will land in another PR.

* Remove snaps that should be regenerated.

* Don't show labels when no x or y specified (to match existing behavior).

* Update snapshots (need to regenerate)

* Fix bug where x axis was sometimes quantitative rather than temporal.

* Implement st.scatter_chart

* Fix x enc when no x_column.

* Add more scatter tests

* Fix tests.

* Revert to melting data by hand since Snowflake doesn't include Altair 4.2

* Improve error messages.

* Fix MyPy types

* Update snapshots

* Fix empty check

* Revert setup.py

* Remove dead code.

* simple rename

* Remove more dead code

* Address comments.

* Address comments.

* Updating screenshot

* Fix snapshots

* Fix stupid copy/paste bug >_<

* Fix stupid copy/paste bug >_<

* Fix merge issues

* Add snapshots

* Small cleanup. Always use str() around dataset ids. Should be a no-op since JSON stringifies keys anyways, but this is cleaner.

* Fix doc bug.

* REALLY fix the doc bug

* Improve docstrings

* Address comments. Move legend settings to CustomTheme.tsx

* Rename test images.

* Address comments. Fixed test.

* Undo move of legend config to CustomTheme.tsx as it broke existing behavior.

* Fix tests

* Fix legend test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mayagbarnes mayagbarnes mayagbarnes left review comments

@willhuang1997 willhuang1997 willhuang1997 approved these changes

@sfc-gh-wihuang sfc-gh-wihuang Awaiting requested review from sfc-gh-wihuang

Assignees
No one assigned
Labels
change:feature impact:users QA-Done QA is Complete security-assessment-completed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sfc-gh-tteixeira @mayagbarnes @willhuang1997 @sfc-gh-wihuang @kasim-inan