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

Reorganise repo structure and store input data as parquet #50

Merged
merged 41 commits into from
Sep 4, 2024
Merged

Reorganise repo structure and store input data as parquet #50

merged 41 commits into from
Sep 4, 2024

Conversation

milanmlft
Copy link
Member

@milanmlft milanmlft commented Sep 2, 2024 •
edited
Loading

Did some file reorganisation to reduce the dev/ folder.

  • Scripts now live in scripts/
  • Raw data (i.e. OMOP extracts yet to be processed) live in data-raw/
  • Processed data lives in data/, which is build ignored so it's not part of the package, so we can mount this folder in the Docker container and live update the data while the app is running (see Set up deploy environment #45 (comment))
  • Renamed inst/test_data/ to inst/dev_data/ to make it more explicit that this is dummy data used for development

Fixes #28
Fixes #34

milanmlft added 30 commits August 30, 2024 14:11
@milanmlft
Move raw data to data-raw/ and scripts to scripts/
d76c5a0
@milanmlft
Add master script to generate dummy data
da302a2
@milanmlft
Move envvar declarations to master script
f83ccbf
@milanmlft
Put all scripts in top-level
6223faf
@milanmlft
Rename scripts
ea5bc14
@milanmlft
Use correct name in here::i_am()
ba7e245
@milanmlft
Rename inst/test_data to inst/dev_data
5ddac93 
Declares the intent more accurately
@milanmlft
Small script updates
e846e87
@milanmlft
Enable markdown support for roxygen
282b7bd
@milanmlft
Move common functions into the package
8cbc2df
@milanmlft
Roxygenise
391f380
@milanmlft
Fix paths to dev_data
87d7530
@milanmlft
Rename
51f2110
@milanmlft
Extract calculate_monthly_counts helper and move into package
d799335
@milanmlft
Add simple tests for monthly counts
a5a6fa8
@milanmlft
Roxygenise
8bc9f86
@milanmlft
Fix: don't generate duplicate rows for monthly counts
454999c 
This also simplifies `calculate_monthly_counts()`
@milanmlft
Test a sligthly more complex example
cf94247
@milanmlft
Test that calculate_monthly_counts can handle database-stored tables
860826b
@milanmlft
Reduce imports
1aec829
@milanmlft
Use correct function
6a12dce
@milanmlft
Add dbplyr as dev dep for tests
47a688c
@milanmlft
Move summary stat helpers into package
3cdee64
@milanmlft
Roxygenise
77c7f68
@milanmlft
Update script
a91a173
@milanmlft
Add tests for summary stats
bc84152
@milanmlft
Update README
6f21f5d
@milanmlft
Add RSQLite as dev dependency
3c423b5
@milanmlft
Write summary tables to parquet instead of adding to database
21a63fe 
See
#45 (comment)
@milanmlft
Rename script, avoid confusion between dev and test data
abf0c01
@milanmlft milanmlft requested a review from a team September 2, 2024 17:00
@milanmlft milanmlft marked this pull request as ready for review September 2, 2024 17:00
BaptisteBR
BaptisteBR reviewed Sep 3, 2024
View reviewed changes
BaptisteBR
BaptisteBR reviewed Sep 3, 2024
View reviewed changes
Copy link
Contributor

@BaptisteBR BaptisteBR left a comment

Choose a reason for hiding this comment

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

We probably need to work together to merge the "low-frequency-stats" branch to this one

@milanmlft
Copy link
Member Author

We probably need to work together to merge the "low-frequency-stats" branch to this one

Possibly, can you open a PR already for your branch? And if you're happy with my replies to your comments above, can you approve this PR? We can then deal with any merge conflicts when the time comes.

BaptisteBR
BaptisteBR approved these changes Sep 4, 2024
View reviewed changes
Copy link
Contributor

@BaptisteBR BaptisteBR left a comment

Choose a reason for hiding this comment

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

It seems of for me.

@milanmlft milanmlft merged commit 1183389 into main Sep 4, 2024
2 checks passed
@milanmlft milanmlft deleted the milanmlft/reorganise-repo branch September 4, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BaptisteBR BaptisteBR BaptisteBR approved these changes

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

Successfully merging this pull request may close these issues.

Move pre-processing helper functions into the main package Review file organisation
2 participants
@milanmlft @BaptisteBR