Skip to content
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

Km compactify rectify #101

Merged
merged 114 commits into from
Jul 29, 2022
Merged

Km compactify rectify #101

merged 114 commits into from
Jul 29, 2022

Conversation

kenmawer
Copy link
Contributor

I rectified compactify by overhauling it.

kenmawer added 30 commits May 13, 2022 16:08
@kenmawer
Added some details for compactify
636f212
@kenmawer
Added compactify variable with check.
05a4804
@kenmawer
Updated function.
7760ce4
@kenmawer
Put !!! to indicate incomplete part.
fe1f2d8
@kenmawer
Shortened code
1268bb7
@kenmawer
More code updates, including updates for vignette.
56859cb
@kenmawer
Put in new changes to vignette.
bd4daf3
@kenmawer
Merge branch 'main' of https://github.com/dajmcdon/epiprocess into km…
a6d6975 
…-compactify
@kenmawer
Added function to remove LOCF. This was copy-pasted while working out…
084bf8c 
…side this project.
@kenmawer
Still need to figure out how to get this to not give me a null dataset.
d6a5fed
@kenmawer
Fixed a typo on the code. I still haven't finished it.
87c487d
@kenmawer
Changed some text and fixed a typo.
22937ae
@kenmawer
Add a comment about code
7310d22
@kenmawer
Put in a code stub for compactify testing.
5eb4bdd
@kenmawer
Made tests and fixed a bug.
82756d5
@kenmawer
There's still work to do in ensuring the LOCF rows are printed properly!
97a2161
@kenmawer
Fixed tests, LOCF rows still need to be printed.
21d3d55
@kenmawer
Added warnings for LOCF.
ffb32fe
@kenmawer
Updated testing and warnings.
fd1c26a
@kenmawer
Updated testing.
cbdf9dd
@kenmawer
Still needs updating so tests pass. No longer using delphi.epidata.
c417f56
@kenmawer
Changed how LOCF checking will work. (Still needs work.)
fef7377
@kenmawer
Added updates and changed some names.
b142442
@kenmawer
Changed LOCF check
f7b353b
@kenmawer
Updated archive code.
46628ca
@kenmawer
Improved LOCF filtering by treating NA's as if they are LOCF's due to…
5479dd8 
… being redundant.
@kenmawer
Modified arrange function to also account for other_keys.
4f0ee0d
@kenmawer
Update comment on RStudio.
5420261
@kenmawer
Updated LOCF checking to take out redundancies.
c66cb4e
@kenmawer
Greatly improved as to actually test LOCF with more control over valu…
0a924be 
…es as to check if it works.
@brookslogan brookslogan mentioned this pull request Jul 25, 2022
Open
@brookslogan
Copy link
Contributor

Closes #111. Closes #136.

@brookslogan
Copy link
Contributor

Closes #62.

kenmawer
kenmawer commented Jul 25, 2022
View reviewed changes
Copy link
Contributor Author

@kenmawer kenmawer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me. I'd say that on a separate PR, we can specify examples, etc. on vignettes about what things such as clobberable versions and the end version mean, if those aren't there.

@kenmawer kenmawer requested a review from brookslogan July 25, 2022 18:07
@brookslogan
Copy link
Contributor

Okay, discussing with Ryan, we might need to roll back some of my changes, at least.

@kenmawer
Copy link
Contributor Author

kenmawer commented Jul 25, 2022 •
edited
Loading

The pull request is filled with random changes, which makes it hard to track down what had actually been changed within this pull request rather than outside of it. I may have overlooked something that should be updated/fixed, especially here with the clutter.

@brookslogan
Copy link
Contributor

The Files changed helps with this, or using commit1..commit2 or commit1...commit2 with git commands. Of course, it probably would have been better if for some of these incidental bugfixes if I just made checks for the edge cases they addressed and complained and made the fixes in separate PRs. Then we'd be able to merge more quickly and have less changes from merge conflict resolution to deal with.

To try to prevent this from continuing to get out of hand, I'll try to do the minimal immediate edits from Ryan's feedback and file others in issues.

lcbrooks and others added 4 commits July 27, 2022 04:12
@lcbrooks
Change "LVCF" back to "LOCF", but with consistent intro text
8271a97 
Always introduce version-wise LOCF as "last observation of each observation
carried forward (LOCF)" or something quite similar.

Clarify time-wise LOCF in one place.
@lcbrooks
Rename observed_versions_end[_conflict] -> versions_end[_conflict]
155ea73
@lcbrooks
Rename versions_end_conflict -> sync, "stop" -> "forbid"
a71c3fe
@kenmawer
Merge branch 'main' into km-compactify_rectify
4c1f7c8
@kenmawer
Copy link
Contributor Author

kenmawer commented Jul 27, 2022 •
edited
Loading

It noticed that by fixing the conflicts, the tests fail. Did I wrongly use the bottom part rather than the top part of the conflict?

kenmawer added 2 commits July 27, 2022 11:01
@kenmawer
Updated km-compactify_rectify whilst dealing with conflicts.
2ddc3d5 
Merge branch 'main' of https://github.com/cmu-delphi/epiprocess into km-compactify_rectify

# Conflicts:
#	R/archive.R
#	tests/testthat/test-methods-epi_archive.R
@kenmawer
Empty commit.
c5eaf14 
Merge branch 'km-compactify_rectify' of https://github.com/dajmcdon/epiprocess into km-compactify_rectify

# Conflicts:
#	R/archive.R
#	tests/testthat/test-methods-epi_archive.R
@brookslogan brookslogan mentioned this pull request Jul 27, 2022
Open
@brookslogan
Copy link
Contributor

I'm looking at git log --remerge-diff and some minor point did pop up. I'll push changes for what I notice, then you can double-check. After that, maybe I'll just file the remaining issues Ryan brought up in Issues and merge so we don't have to keep merging main in here.

@brookslogan brookslogan merged commit b90916d into cmu-delphi:main Jul 29, 2022
This was referenced Aug 1, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brookslogan brookslogan brookslogan approved these changes

@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.

Add option to compactify issue data in epi_archive
4 participants
@kenmawer @brookslogan @dajmcdon @lcbrooks