Improving aggregate #47
Conversation
- see news
removing version from is_installed to fix bug introduced with new R version
fixing test issue caused by converting covariateSettings to a list
see news
fixing checks
fixing example check issues
- removed pdf from package as user can access via website - updated website - renamed vignette called UsingCharacterizationPackage to UsingPackage as name too long for window's R check
updating aggregate characterization to use custom during covariates for case analysis
| on t.subject_id = o.subject_id | ||
| where | ||
| -- outcome starts before TAR end | ||
| o.cohort_start_date <= dateadd(day, @tar_end, t.@tar_end_anchor) | ||
| -- outcome starts and ends within prior_outcome_washout_days |
There was a problem hiding this comment.
The comment says 'starts and ends' but I think it only matters where the outcome starts. Your code is only checking start is between washout and 1 day before the cohort start date, which I think is logically correct, the comment is just misleading.
There was a problem hiding this comment.
yeah - I think originally I was wondering whether to write logic to remove if outcome overlaps with washout days, but decided to just do whether it starts during washout days, so the comment is not correct. We can remove 'and ends'.
…inations.sql Fixed case sensitivity
Changed case of CreateTargetOutcomeCombinations.sql
…terization into improving_aggregate
fixing primary key for test to pass
Improving aggregate results model
removing migration and package_version from exports
|
Ok, this is all good, when we get to the pint where we merge into develop, let's use 'sqash and merge' and combine the commits (and commit message) into one commit comment so that it doesn't spam commit history with a lot of noise. I can manage the commit into develop when the time comes, plese let me know when it's ready and I can do it (You can reach me on teams) |
- added columns to PK for dechal-rechal table - added max rows to export input when doing csv export
- supporting multiple TARs - split aggregate covariate SQL into parts with and without TAR - added time_to_event result table with migrations
fixing time_at_risk tables
- adding checks for unique cohort_definition_ids
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #47 +/- ##
===========================================
+ Coverage 86.99% 87.22% +0.22%
===========================================
Files 8 9 +1
Lines 1384 1800 +416
===========================================
+ Hits 1204 1570 +366
- Misses 180 230 +50 ☔ View full report in Codecov by Sentry. |
- using row_id for FE to not have conflicts - adding hash of settings to be runId for uniqueness and for incremental mode
Before you do a pull request, you should always file an issue and make sure the package maintainer agrees that it’s a problem, and is happy with your basic proposal for fixing it. We don’t want you to spend a bunch of time on something that we don’t think is a good idea.
Additional requirements for pull requests:
Adhere to the Developer Guidelines as well as the OHDSI Code Style.
If possible, add unit tests for new functionality you add.
Restrict your pull request to solving the issue at hand. Do not try to 'improve' parts of the code that are not related to the issue. If you feel other parts of the code need better organization, create a separate issue for that.
Make sure you pass R check without errors and warnings before submitting.
Always target the
developbranch, and make sure you are up-to-date with the develop branch.