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

Edit thresholds #4

Merged
merged 13 commits into from
May 17, 2021
Merged

Edit thresholds #4

merged 13 commits into from
May 17, 2021

Conversation

egarcialara
Copy link

@egarcialara egarcialara commented May 4, 2021

Overkill? maybe. But now it's done 🤷‍♀️

@egarcialara egarcialara marked this pull request as ready for review May 6, 2021 10:58
Copy link

@MaximMoinat MaximMoinat left a comment

Choose a reason for hiding this comment

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

The good: it works!
The less good: it is a bit 'unwieldly'. Still requires a lot of manual lookup work. I have to say, it is better than it was in the original, wide thresholds file. But I think we can make it a lot easier for the user. Let's discuss.

extras/edit_thresholds/edit_thresholds_aux.R Outdated Show resolved Hide resolved
Comment on lines 8 to 17
# 1) get longer table
df_long <- pivot_longer_func(file)
write.csv(df_long, file.path(getwd(), "dflong.csv"))

# 2) Edit thresholds, in 'df_long' directly or in excel file 'dflong.csv'
# using the indexes selected using file_thresholds & printed here:
# (be aware that the excel file might have one extra row for column names, see the index in the first column!!)
file_thresholds <- "thresholds_to_add.csv"
print_threshold_location(df_long, file_thresholds)

Choose a reason for hiding this comment

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

This feels like double work; first creating a file with what thresholds you want to change, then looking it up in dflong.csv, and entering the threshold percentage.

Ideally you already add the threshold in the input file:

Level Check_name cdmTableName cdmFieldName fkTableName fkDomain threshold note
Field isRequired MEASUREMENT person_id 35

Then we can also automate steps 2+3, where the script first looks up the index of the check and then fills in the threshold and note.

Copy link
Author

Choose a reason for hiding this comment

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

I avoided it so that the Notes don't get duplicated in the case of multiple runs. It's corrected now

Choose a reason for hiding this comment

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

How would notes get duplicated? The way I see it, an existing note in this file can just be updated in a next run.

extras/edit_thresholds/edit_thresholds.R Outdated Show resolved Hide resolved
extras/edit_thresholds/README.md Outdated Show resolved Hide resolved
extras/edit_thresholds/README.md Show resolved Hide resolved
extras/edit_thresholds/README.md Outdated Show resolved Hide resolved
@egarcialara
Copy link
Author

egarcialara commented May 7, 2021

Oh la la!
Changes made:

  • running functions automatically. Indices remain as a variable and can be print if desired
  • running for 3 fields at the same time -and with the same thresholds file- supported
  • simplified readme
  • small changes: checkName, the way of printing the comparison of old/new tables

Copy link

@MaximMoinat MaximMoinat left a comment

Choose a reason for hiding this comment

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

Looks good, works for my limited test (only on field level).
And looking forward to test this out in a next project!

@egarcialara egarcialara merged commit c3cbbf0 into utilities May 17, 2021
@egarcialara egarcialara deleted the edit_thresholds branch May 17, 2021 15:21
MaximMoinat pushed a commit that referenced this pull request Apr 13, 2022
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