Skip to content

epi_slide and epix_slide args #510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dajmcdon opened this issue Aug 13, 2024 · 3 comments · Fixed by #513 or #477
Closed

epi_slide and epix_slide args #510

dajmcdon opened this issue Aug 13, 2024 · 3 comments · Fixed by #513 or #477
Assignees

Comments

@dajmcdon
Copy link
Contributor

I still find before/after confusing. It takes mental math and careful reading of the documentation. It's more general, but not really how our target users are likely to think.

I suggest we bring back n for the window size as well as align = c("left", "center", "right"). We can keep before/after as NULL and ensure there aren't any conflicts. Then immediately process n+align into the appropriate before+after pair.

@brookslogan
Copy link
Contributor

brookslogan commented Aug 14, 2024

Thanks for the ping. I think this fell down the stack due to some engineering priorities + new testers getting tripped up by other things. Pinging @dshemetov here in case he's interested in tackling; seems related to time type rework. [Otherwise I can take a look after my other currently-planned slide edits.]

@dajmcdon have you ever used / remember us using something that's not left/center/right aligned? I know we've used non-odd-n center-aligned things at least (e.g., before = 6, after = 7), but that's the closest I can think of.

Context/history for implementer:

  • We didn't move to before & after for generality; epi_slide actually had an n + align + before [with the latter two mutually exclusive]; this allowed for the same level of generality as before + after. The actual reason for the change had to deal with [epix_slide stuff; described at end] + trying to mirror interfaces between epix_slide and epi_slide; the latter's no longer a key goal.
    • Plan since this summer has been to urgently change back to n + align + before.
    • Though n + before seems awkward; I like @dajmcdon's suggestion of n + align + before + after.... assuming we want to support this level of generality at all.
  • We should make sure to mirror @dshemetov's time type type rework rules when implementing this change.
  • (History: how did epix_slide make [us] change epi_slide? n was always misleading in epix_slide to some people (and sometimes misleading in epi_slide, though mirroring the recent time type rework should take care of some cases [without needing time_step / allowing silently-buggy/confusing alternatives]; the remaining cases are the continually-frustrating/complicated cases with [edges and gaps of the data set, and] with non-epikey-grouped epi_slides with differing epikey availability), so we changed to epix_slide(before =) and, for consistency, epi_slide(before =, after =).)

@dshemetov
Copy link
Contributor

Happy to tackle this, should be pretty simple. I don't have strong opinions on the interface here, it's all equivalent to me.

@dshemetov
Copy link
Contributor

See #513 for a simple prototype.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants