Skip to content

allow dummy axes for relevant layers#442

Open
thomasp85 wants to merge 2 commits intomainfrom
issue-440-dummy-axes
Open

allow dummy axes for relevant layers#442
thomasp85 wants to merge 2 commits intomainfrom
issue-440-dummy-axes

Conversation

@thomasp85
Copy link
Copy Markdown
Collaborator

Fix #440

This PR also removes the needs_stat_transform() trait method since it is no longer being used (all layers goes through a stat phase)

@thomasp85 thomasp85 requested a review from teunbrand May 8, 2026 13:08
Copy link
Copy Markdown
Collaborator

@teunbrand teunbrand left a comment

Choose a reason for hiding this comment

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

I'm sorry to keep pushing on this point again, but do you think we could make more use of the pre-existing infrastructure for this?

For example, I think the optional positions might have DefaultAestheticValue::String() with the dummy value. I'd be happy to have position literals materialised as proper columns somewhere at the beginning if that is the bottleneck. The precedent for this is process_annotation_layer also treating position literals similarly.

@thomasp85
Copy link
Copy Markdown
Collaborator Author

I'm happy you push back, but I'm not sure I see an upside to your proposal... If we use a string sentinel as default then wouldn't the user be able to inadvertently provide that in a plot and be surprised. To me, Null seems like the right choice

@teunbrand
Copy link
Copy Markdown
Collaborator

We can also expand the enum with DefaultParamValue::Dummy if strings are the problem. I'm not sure I understood the scenario correctly, you'd mean that the user would write MAPPING '__ggsql_dummy_' AS x and be surprised that the axis get surpressed?

@thomasp85
Copy link
Copy Markdown
Collaborator Author

yeah, basically... I think it is bad design to let this seep out however unlikely

I'm fine with adding an explicit dummy instead

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.

Allow dummy axis in violin, point, range, boxplot

2 participants