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.

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

Add names for bundles and test they are consistent #17

Merged
merged 9 commits into from
Sep 12, 2024

Conversation

stefpiatek
Copy link
Contributor

Moved raw data to new structure with a separate file for the names, and added in some tests to ensure that we can detect inconsitencies. Bit of refactoring of raw data functions, expect that once we have a release serving off a package .rda file that we'll change the raw data to live outside of the package but seems fine for now as we still need to serve from the package.

  • One question though, what domain does indices of deprivation go into?

@milanmlft
Copy link
Member

expect that once we have a release serving off a package .rda file that we'll change the raw data to live outside of the package but seems fine for now as we still need to serve from the package.

Agreed, having it run off .rda would mean all the data is immediately available and we don't need to re-read the csv files every time we wan to access a bundle. On the other hand, it might make loading the package slower as all the data would immediately be loaded in as well. If the data gets big enough that might become a performance issue. Maybe worth doing some benchmarking on that once we get to that point.

Copy link
Member

@milanmlft milanmlft left a comment

Choose a reason for hiding this comment

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

Not sure why the ::: everywhere for get_raw_dir()?

R/bundles.R Outdated Show resolved Hide resolved
R/bundles.R Outdated Show resolved Hide resolved
R/bundles.R Outdated Show resolved Hide resolved
R/raw-data.R Outdated Show resolved Hide resolved
R/raw-data.R Show resolved Hide resolved
R/raw-data.R Show resolved Hide resolved
tests/testthat/test-raw-data.R Outdated Show resolved Hide resolved
tests/testthat/test-raw-data.R Outdated Show resolved Hide resolved
tests/testthat/test-raw-data.R Outdated Show resolved Hide resolved
tests/testthat/test-raw-data.R Outdated Show resolved Hide resolved
Copy link
Member

@milanmlft milanmlft left a comment

Choose a reason for hiding this comment

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

Shouldn't have to call library() or omopbundles::* in tests, as the package code should work independently of that

tests/testthat/test-bundles.R Outdated Show resolved Hide resolved
tests/testthat/test-bundles.R Outdated Show resolved Hide resolved
Copy link
Member

@milanmlft milanmlft left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀 Just some suggestions regarding library() and ::/::: calls
Just wondering how the bundle_names.csv files get generated? Might be worth adding a script to reproduce these files to the package. A common place for this could be data-raw/bundle_names.R

@stefpiatek
Copy link
Contributor Author

Just wondering how the bundle_names.csv files get generated? Might be worth adding a script to reproduce these files to the package. A common place for this could be data-raw/bundle_names.R

I think the names should be manually curated, or exported from the source system

@stefpiatek stefpiatek merged commit c9f786a into main Sep 12, 2024
2 checks passed
@stefpiatek stefpiatek deleted the stef/check-raw-data branch September 12, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants