Refactor vega-related charting #8595
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| return self.dg._enqueue( | ||
| "arrow_area_chart", proto, add_rows_metadata=add_rows_metadata | ||
| return self._altair_chart( |
There was a problem hiding this comment.
nit: it might be a good idea to leave a small comment about how Vega and Altair belong together, because the class is called VegaChartsMixin but then here we return an altair_chart. This could be confusing to people who do not know about these projects
There was a problem hiding this comment.
Added a docstring comment to the VegaChartsMixin class to explain how the commands are related 👍
| out_data: Data | ||
|
|
||
| # For some delta types we have to reshape the data structure | ||
| # For built-in chart commands we have to reshape the data structure |
There was a problem hiding this comment.
nit: if this function is only relevant for chart commands but not generally for everything that uses arrow / add-row functionality as the comment says, I suggest to rename the function to sth. like _prep_chart_data_for_add_rows and perhaps move it to the new built_in_chart_utils.py
There was a problem hiding this comment.
We cannot easily fully move this method since its used in all add row cases, but I extracted the chart specific logic to built_in_charts_utils.
There was a problem hiding this comment.
alrighty, but is this function only relevant for chart types or in general?
There was a problem hiding this comment.
The migrated part is only relevant for built-in charts, but _prep_data_for_add_rows is called in all add row cases.
| color_column: str | None, | ||
| size_column: str | None, | ||
| ) -> tuple[pd.DataFrame, str | None, str | None]: | ||
| """If multiple columns are set for y, melt the dataframe into long format.""" |
There was a problem hiding this comment.
nit: could this comment be extended with an example? I am not sure what it means to melt a dataframe into long format
There was a problem hiding this comment.
Added more info to the comment
Describe your changes
This PR refactors/restructures our backend logic that handles all vega-related charting:
st.vega_lite_chart,st.altair_chart,st.line_chart,st.area_chart,st.bar_chart,st.scatter_chart.built_in_chart_utilsvega_chartsand refactored to reuse more shared logic.add_rowsmetadata by just relaying on the available metadata instead of checking for the delta type.Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.