Skip to content

Updated epi_slide to use before and after and added checks #188

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 43 commits into from
Nov 13, 2022
Merged

Updated epi_slide to use before and after and added checks #188

merged 43 commits into from
Nov 13, 2022

Conversation

kenmawer
Copy link
Contributor

@kenmawer kenmawer commented Aug 6, 2022

Note that epix_slide has been untouched.

Resolves issue #161.

kenmawer added 17 commits July 26, 2022 15:07
@kenmawer
Some cleanup of slide; still incomplete.
bb39209
@kenmawer
Still needs changes as before and after numbers are wrong.
5984d8d
@kenmawer
Changed bad formatting.
b68af0b
@kenmawer
Still needs refactoring.
b3229f2
@kenmawer
Redocumented with changes; still needs changes.
d18d98c
@kenmawer
Bad changes that break things.
121f9d2 
Merge branch 'main' of https://github.com/cmu-delphi/epiprocess into km-slide-n-replace

# Conflicts:
#	R/slide.R
#	man/epi_slide.Rd
@kenmawer
Seems like merge is broken.
35811f1
@kenmawer
Merge branch 'main' of https://github.com/cmu-delphi/epiprocess into …
37b3815 
…km-slide-n-replace2
@kenmawer
Merge branch 'km-slide-n-replace' of https://github.com/dajmcdon/epip…
f6e8795 
…rocess into km-slide-n-replace2
@kenmawer
Seems broken beyond repair.
ee10963
@kenmawer
Fixed tests.
05d84ca
@kenmawer
Fixed improper use of n.
846b6ca
@kenmawer
This finally runs without errors.
d55e6b8
@kenmawer
Note that epix_slide still hasn't been updated, and some epi_slide do…
1158c8a 
…cumentation still neneds to be modified.
@kenmawer
Need to ensure tests pass.
bbf5d6b
@kenmawer
This shouldn't be here.
b55d411
@kenmawer
Removed repetitive code and added more tests.
b22ace3
brookslogan
brookslogan requested changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Main requested changes: tweaks to the documentation to try to clarify a bit. I've also noted some edge cases not caught by the before&after validation, seem pretty low-probability but while it's on our minds probably should add the checks. + some misc minor things.

Please address + re-request review (and maybe ping me on Slack). Remaining TODO on my end: double-check that we've stamped out all references to n and align in code, docs, tests, examples, vignettes.

@kenmawer kenmawer requested a review from brookslogan August 10, 2022 20:28
@kenmawer kenmawer mentioned this pull request Aug 15, 2022
Closed
@kenmawer
Copy link
Contributor Author

kenmawer commented Aug 15, 2022 •
edited
Loading

Note: #203 should be used after this due to the former also incorporating changes to epix_slide.

@kenmawer
Copy link
Contributor Author

Add case for centre alignment.

dajmcdon
dajmcdon requested changes Aug 16, 2022
View reviewed changes
@kenmawer kenmawer requested a review from dajmcdon August 16, 2022 21:57
dajmcdon
dajmcdon reviewed Aug 17, 2022
View reviewed changes
Copy link
Contributor

@dajmcdon dajmcdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor bit of confusion, and one comment to be approved by @brookslogan .

@brookslogan brookslogan mentioned this pull request Aug 18, 2022
Closed
@brookslogan
Copy link
Contributor

brookslogan commented Aug 18, 2022 •
edited
Loading

Remaining TODO on my end: double-check that we've stamped out all references to n and align in code, docs, tests, examples, vignettes.

  • I just did a cursory search (rg (ripgrep)-ing for "n"). It looks like the intro paragraph to slide.Rmd still needs updated. [Done]

brookslogan
brookslogan reviewed Aug 19, 2022
View reviewed changes
@kenmawer kenmawer requested a review from brookslogan August 20, 2022 00:31
@lcbrooks
Require >=1 of before,after; ensure time_step receives integer
9f5ee8c 
- Raise error, not warning, if both `before` and `after` are missing
- Substantially reword docs for `before, after`
- Actually deliver an integer to `time_step` as currently advertised
- Slightly loosen and rearrange validation of `before`, `after` to ease
  conversion to integer.  Looseness is from allowing things that can be cast to
  appropriate integers, rather than requiring integer-like numerics.
@lcbrooks
Format epi_slide roxygen examples
0fec3ae
@lcbrooks
Fix some outdated docs, refine wording on others
d9682da 
- Remove reference to `n` in slide vignette intro.
- Update .Rd files with some older roxygen changes.
- Fix "`time_value`s" -> "time steps" in `detect_outlr_rm` docs, and slightly
  tweak wording.
@brookslogan
Copy link
Contributor

brookslogan commented Aug 23, 2022 •
edited
Loading

Maybe Dan TODO:

  • Sanity-check the before&after roxygen docs I substantially changed

My TODOs:

  • Is before and after validation too strict? We may want to form running windows by using, e.g., before=Inf. May want to allow Inf here, and update the time_step docs to be match. Furthermore, check whether Inf should be the default instead of 0 in some places. Consider compatibility with epix_slide's implicit after=Inf. (And epix_slide could reasonably have a default before=Inf while epi_slide could not.) Inf might not work right now if it were allowed; allowing Inf could be a separate issue. (Sys.Date()-Inf prints as NA, but actually, as.numeric(Sys.Date()-Inf) is -Inf, so maybe it would work if we kept the validation loose and didn't use a custom time_step?)
  • Check the pkg checks
  • Implement idea from Periodically bump version, perform GitHub release, maintain news/changelog #177 before merging.

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

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

@brookslogan brookslogan merged commit 8b00b3b 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

@elray1 elray1 elray1 left review comments

@dajmcdon dajmcdon dajmcdon requested changes

@brookslogan brookslogan Awaiting requested review from brookslogan

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

Successfully merging this pull request may close these issues.
5 participants
@kenmawer @brookslogan @elray1 @dajmcdon @lcbrooks