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

[Demo] Add lollipop chart to ViViVo #874

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

huong-li-nguyen
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen commented Nov 14, 2024

Description

Based on #787 - thank you again yonkmanjl! ⭐

  • Adds refactored code for lollipop chart based on px.scatter

Screenshot

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

@huong-li-nguyen huong-li-nguyen self-assigned this Nov 14, 2024
Copy link
Contributor

github-actions bot commented Nov 14, 2024

View the example dashboards of the current commit live on PyCafe ☕ 🚀

Updated on: 2024-11-15 10:42:05 UTC
Commit: 5dda70d

Link: vizro-core/examples/dev/

Link: vizro-core/examples/scratch_dev

Link: vizro-core/examples/visual-vocabulary/

Link: vizro-ai/examples/dashboard_ui/



@capture("graph")
def lollipop(data_frame: pd.DataFrame, x: str, y: str):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old code for reference

Copy link
Contributor

Choose a reason for hiding this comment

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

The new code is much better 🥇

Comment on lines +333 to +336
# Unlike the column_and_line chart, where all traces hold equal significance, here the traces differ in importance.
# The primary scatter plot is the main trace, while the additional traces merely serve as connecting lines.
# Therefore, should we apply the kwargs solely to the main scatter plot, as illustrated below?
fig = px.scatter(data_frame, **kwargs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One question: In this case, there is one main scatter trace and several subsidiary scatter traces that only add lines. I assume in this case, we only add kwargs to the main trace and we don't enable the the kwargs for the subsidiary scatter traces?

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume in this case, we only add kwargs to the main trace and we don't enable the the kwargs for the subsidiary scatter traces?

I think this is totally correct 👍.

)

fig.update_layout(
showlegend=False, yaxis_showgrid=yaxis_showgrid, xaxis_showgrid=xaxis_showgrid, yaxis_rangemode="tozero"
Copy link
Contributor Author

@huong-li-nguyen huong-li-nguyen Nov 15, 2024

Choose a reason for hiding this comment

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

I'm wondering if we should generally include layout customizations that optimize a given chart but might not be applicable depending on the data. For instance, setting yaxis_rangemode="tozero" works well for positive data as it removes the gap between the x-axis zeroline and the start of the y-axis, but it's not suitable for negative data.

Additionally, I've just realized that our chart code might not be dynamic enough to handle negative data. Specifically, the calculation code from lines 343-350 does not account for negative values. This might also be the case with other charts, which led me to the question: Does it have to? What's our minimum requirement?

I think we need to refine the minimum requirements a code snippet for the ViViVo needs to satisfy, which is different from a chart that lives in vizro.charts (tbd whether we actually do that). Some of these include:

Potential requirements

  • Needs to handle both wide and long form data (Yes)
  • Needs to handle both orientations (This depends on the chart type for me - for the lollipop I would have said yes, for the diverging stacked bar chart I would have said: nice to have, but not really necessary)
  • Needs to handle negative data
  • Needs to be optimized for data viz best practices (this degree can also vary)
  • ...

Personally, I see the visual-vocabulary as a guide for best practices in data visualization. I don't mind if some aspects are hard-coded and if the chart doesn't handle all scenarios, as it's meant to be a starting code snippet that users can extend. Trying to handle all of these scenarios will add complexity and reminds me of old VizX times, where we had a bunch of if/else inside the chart code dealing with all possible scenarios that might not be relevant to all users.

vizro-core/examples/visual-vocabulary/custom_charts.py Outdated Show resolved Hide resolved
vizro-core/examples/visual-vocabulary/custom_charts.py Outdated Show resolved Hide resolved
Comment on lines +333 to +336
# Unlike the column_and_line chart, where all traces hold equal significance, here the traces differ in importance.
# The primary scatter plot is the main trace, while the additional traces merely serve as connecting lines.
# Therefore, should we apply the kwargs solely to the main scatter plot, as illustrated below?
fig = px.scatter(data_frame, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume in this case, we only add kwargs to the main trace and we don't enable the the kwargs for the subsidiary scatter traces?

I think this is totally correct 👍.



@capture("graph")
def lollipop(data_frame: pd.DataFrame, x: str, y: str):
Copy link
Contributor

Choose a reason for hiding this comment

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

The new code is much better 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants