Skip to content

Tidy multiple_marks.py#2640

Merged
joelostblom merged 4 commits intovega:masterfrom
palewire:patch-15
Jul 5, 2022
Merged

Tidy multiple_marks.py#2640
joelostblom merged 4 commits intovega:masterfrom
palewire:patch-15

Conversation

@palewire
Copy link
Contributor

@palewire palewire commented Jul 1, 2022

This chart doesn't have a clear purpose, and it is duplication of https://altair-viz.github.io/gallery/line_chart_with_points.html

So I propose we cut it.

@joelostblom
Copy link
Contributor

I agree that there is overlap between these two from a documentation perspective. However, the gallery examples are also used as a test suite for Altair, so there is an advantage from that angle to include both since they test point=True and point=alt.OverlayMarkDef(), respectively. But maybe we could make them slightly more different so that it make sense from a documentation perspective as well?

@palewire
Copy link
Contributor Author

palewire commented Jul 3, 2022

Does point=True translate to point=alt.OverlayMarkDef() under the hood inside of Altair internals? If that's the case, substituting it in for the current example would result in the code being "covered" in both instances, which would address your testing concern without clinging to a cruft example. It also would result in a simpler example for newbies. What do you think?

@joelostblom
Copy link
Contributor

It it not translated in Altair, but sent to Vega-Lite which also accepts True or a definition. So point=True is sent as 'mark': {'type': 'line', 'point': True}, whereas point=alt.OverlayMarkDef(color="red") is sent as 'mark': {'type': 'line', 'point': {'color': 'red'}}. There are also two corresponding examples in the VegaLite gallery here and here so I think we should keep two in the Altair examples too where the basic one just shows how to add points and the more advanced how to modify their appearance.

@palewire
Copy link
Contributor Author

palewire commented Jul 5, 2022

I've revised this proposal to satisfy your request and bring out examples into harmony with what's in the Vega-Lite documentation. It conflicts a bit with #2644 but that's okay by me. We can always revise that request after we settle this one.

@palewire palewire changed the title Delete multiple_marks.py Tidy multiple_marks.py Jul 5, 2022
Copy link
Contributor

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the updates!

@joelostblom joelostblom merged commit 74f24b1 into vega:master Jul 5, 2022
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