Skip to content

Switch nested drawing loop: First facets, then by groups#331

Merged
grantmcdermott merged 7 commits intomainfrom
facet-loop-first
Mar 7, 2025
Merged

Switch nested drawing loop: First facets, then by groups#331
grantmcdermott merged 7 commits intomainfrom
facet-loop-first

Conversation

@grantmcdermott
Copy link
Owner

@grantmcdermott grantmcdermott commented Mar 3, 2025

Motivated by various discussions, e.g. #233 (comment), #305 (comment), and #314 (comment).

This PR switches the nested loop that we use for drawing interior plot elements. Specifically, the proposed new nesting logic is:

  1. Outer loop over facets
  2. Inner loop over groups (i.e., by within each facet)

The old nesting logic inverted this relationship—groups first, then facets—but this was mostly just an artifact of historical inertia. (I only implemented facet support much later than group support, which was enabled from the get-go.) In addition, the whole datapoints + dedicated types refactoring (#222) has now made it much easier to keep track of the final state of transformed variables (e.g., by variables that are created internally as part of the relevant type_draw function). All told, looping over facets first seems the most natural way to do things now.

All tests are passing locally for me, but @zeileis and @vincentarelbundock please take a look if you can. Achim, I should also flag that this will possibly (probably?) break your PR in #314, but hopefully facets first will simplify other parts of type_barplot() support. It may be possible to simplify parts of type_spineplot and type_ridge too, as well as any other types that rely on tinyAxis internally, but I didn't look into this in great detail.

@vincentarelbundock
Copy link
Collaborator

sorry, probably won't be able to look at this, but if the tests pass, I can't think of a problem

@grantmcdermott
Copy link
Owner Author

Okay, I'll merge tomorrow. @zeileis lmk if you'd like me to hold off for a bit first, although we can always revert if something goes awry.

@grantmcdermott grantmcdermott merged commit c31c26b into main Mar 7, 2025
3 checks passed
@zeileis
Copy link
Collaborator

zeileis commented Mar 8, 2025

Grant, this is great, thank you so much for making this effort!

Indeed it broke the code in #314 but this was easy to fix: I just had to replace data_by with data_facet. Recombining the split-up data_facet is now also much easier than the previous code based on data_by, thanks!

@zeileis zeileis mentioned this pull request Mar 8, 2025
@grantmcdermott grantmcdermott deleted the facet-loop-first branch March 9, 2025 14:07
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.

3 participants