Skip to content

Replace n with before (no after) in epix_slide #216

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Nov 13, 2022
Merged

Replace n with before (no after) in epix_slide #216

merged 18 commits into from
Nov 13, 2022

Conversation

kenmawer
Copy link
Contributor

I somehow managed to break the other branch, so I put this in. This does not include random stuff from epi_slide.

brookslogan
brookslogan reviewed Aug 19, 2022
View reviewed changes
@kenmawer kenmawer requested a review from brookslogan August 20, 2022 00:35
@brookslogan brookslogan changed the title Refactor epix_slide Replace n with before in epix_slide Aug 23, 2022
@brookslogan brookslogan changed the title Replace n with before in epix_slide Replace n with before (no after) in epix_slide Aug 23, 2022
@brookslogan
Copy link
Contributor

brookslogan commented Aug 23, 2022 •
edited
Loading

Other TODOs

  • Check if before_num can be refactored out as I believe was done in in epi_slide PR. [It can be refactored out. Done.]

@brookslogan
Copy link
Contributor

brookslogan commented Aug 24, 2022 •
edited
Loading

  • Decide whether/how to fix "#' 1. epix_slide() uses windows that are always right-aligned (in" [actually, the window extends infinitely far right, but in common use cases, data often ends before each ref_time_value; neither really looks exactly like right-alignment] --- [I did adjust this discussion a bit.]

@lcbrooks
Fix local_edition ambiguity in tests
6c56208 
This appears to cause errors when running tests within some interactive
sessions, e.g., from running tests (attaching `testthat`), then
`library(tidyverse)` (attaching `readr` and masking `local_edition` even if
`testthat` is re-`library`-ied in), then attempting to run tests again.
@lcbrooks
Update epix_slide before docs, add validation
d648910 
- Try to describe `before` in a similar manner as the PR for `epi_slide`
  `before`&`after` currently does.
- Fix some example discussion not yet adjusted for `before` being off by one
  relative to `n`.
- Update some discussion about differences between `epix_slide` and `epi_slide`
  `time_value` windows.
- Add tests for the `before` validation code
@brookslogan
Copy link
Contributor

Remaining TODOs:

@brookslogan brookslogan changed the base branch from main to dev November 13, 2022 20:38
@brookslogan
Copy link
Contributor

Merging into dev as is. Remaining items can be addressed separately before creating a new release there.

@brookslogan brookslogan merged commit 245448a into cmu-delphi:dev Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brookslogan brookslogan brookslogan left review comments

@dajmcdon dajmcdon Awaiting requested review from dajmcdon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kenmawer @brookslogan @lcbrooks