grantmcdermott · GitHub

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.

Read the original on github.com ↗