search

r-devel / r-dev-day

Repo to organize tasks for R Dev Days
7 stars 2 forks source link

Bug 8934 - Irregularity in stem() display #12

Closed hturner closed 3 months ago

hturner commented 6 months ago

Bug 8934 describes some long-standing issues with the stem() function for creating stem-and-leaf plots, the main one being that the divider between the stem and the leaf is not always aligned across rows.

The bulk of the code is in the underlying stem_leaf C function: https://github.com/r-devel/r-svn/blob/cd94cda92d838a2938cdb90b51ca1d0cc3eeb7af/src/library/graphics/src/stem.c#L51.

So fixing the alignment bug would require working through this code to understand how the alignment is determined and whether it can changed to make the dividers always line up.

EllaKaye commented 6 months ago

I'm interested in learning more about the C code that underpins R, so I'd be happy to take a look into this, once I've addressed #5, though I might not have a chance to do both on the day at Imperial.

kbodwin commented 4 months ago

@EllaKaye and I have submitted a patch that fixes the main issue of this bug, i.e., the misalignment of the divider.

The two other suggested changes in Bug 8934 are not addressed and could still be good Dev Day issues.

hturner commented 3 months ago

Patch is now in R-devel and bug has been marked as CLOSED FIXED :tada:

EllaKaye commented 3 months ago

When I'm back at work next week, I'll open another bugzilla report regarding the two additional suggested issues from the now closed one. Those are open to discussion, rather than genuine bugs, so good to treat them differently.

mmaechler commented 3 months ago

When I'm back at work next week, I'll open another bugzilla report regarding the two additional suggested issues from the now closed one. Those are open to discussion, rather than genuine bugs, so good to treat them differently.

The "do nothing for n=1" issue is indeed something to be discussed. Remember Di Cook proposed the cutoff should even be, e.g., n <= 5 instead of n = 1. Now bugzilla is really not the best place for discussions; also it is only read by R-core members (+ Heather) generally, plus the PR "subscribers" specifically. Usually I'd use the R-devel mailing list for discussing possible R changes. .. but you could open issues somewhere else and post / blog / chat / .. about it.

And the second issue brought up, the default scale, may be "interesting", but then stem() is not really the most important R function, but rather almost a historical monument, no?

EllaKaye commented 3 months ago

Thanks @mmaechler for advising on the best forum for the discussion. Next week I'll raise this on the R-devel maillist, and possibly a Mastodon chat too.

stem() may be an almost historical monument, but as a chance for newbie contributors to learn about the process and practice working with C code, it's been a really nice little project 😄

pmur002 commented 3 months ago

Thanks to Ella and Kelly for the fix! Nice to see the regression test included in the patch too :)