refactor: ♻️ turn foer_besoegsdag into a single resource#93
refactor: ♻️ turn foer_besoegsdag into a single resource#93joelostblom wants to merge 21 commits into
Conversation
Join vas_minus10 into the combined VAS resource using minutes_from_meal so negative and positive meal-relative timepoints share one schema.
Remove the redundant vas_ prefix from variables after they are moved into the combined VAS resource.
Use descriptive title and description values for the combined VAS resource and make VAS field deduplication easier to read.
Normalize the SEFNC visit-specific REDCap forms into one resource schema with a week field so repeated measurements are represented by rows instead of suffixed columns.
Update the generated data package metadata so SEFNC has one resource with week values 0, 12, and 52 and no visit-suffixed duplicate fields.
Normalize foer_besoegsdag visit-specific forms into one resource schema with a visit field so repeated workflow metadata is represented by rows instead of suffixed resources.
Only remove foer_besoegsdag visit suffixes for the joined visits so descriptions keep meaningful references to other visits and avoid doubled punctuation.
Update generated metadata so foer_besoegsdag visits 2, 3, and 4 share one resource with a visit key and normalized workflow fields.
Build the before-visit field-to-visits map with a reducer so common workflow fields are identified without nested filtering comprehensions.
martonvago
left a comment
martonvago
left a comment
There was a problem hiding this comment.
This makes sense to me in general!
It's a lot of code, but hopefully it will be possible to generalise some of it 🤞 .
github-project-automation
Bot
moved this from In review
to In progress
in Data development
|
This should be re-opened to main, not stacked. Please avoid stacking as much as possible. |
|
I do avoid stacking when I can, but use it when it helps the review process (ie avoid looking at a giant irrelevant diff). In this case, I used it because I was trying to generalize some functionality that would be shared between these PRs, but after deciding steer away from that direction, it would have been better if I removed some of the functionality from here and then unstacked these PRs. No need to close and re-open this though, we can just change the base branch; I just did that and resolved conflicts. |
|
I converted this to draft until #90 has been approved and merged, otherwise it's difficult to know what has been added and what to review. |
joelostblom
left a comment
joelostblom
left a comment
There was a problem hiding this comment.
I think this is ready for another look now that #90 has been merged.
| SEFNC_WEEKS = [0, 12, 52] | ||
| SEFNC_FORM_WEEKS = { | ||
| "sefnc_baseline_v4": 1, | ||
| "sefnc_baseline_v4": 0, |
There was a problem hiding this comment.
I forgot to change this back in the other PR that was just merged. Since this is study week rather than calendar week and the baseline week is included, the range should start at 0
3 participants
Description
This PR combines got a little bit more messy than the other ones because some fields were not in part of each visit and therefore some of them changed from being required to being optional.
But overall this PR does the following:
foer_besoegsdag_2,foer_besoegsdag_3, andfoer_besoegsdag_4into onefoer_besoegsdagresource.visitfield with values2,3, and4, and include it in the primary key withevent._wfv2,_wfv3, and_wfv4suffixes.Visit 2,Visit 3,Visit 4, andWfv2/Wfv3/Wfv4when it only repeats the newvisitfield.Closes #54
This PR needs an in-depth review.
Checklist
just run-all