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

changed value low for days supply to 1 and value high for refills to 24 #528

Merged
merged 3 commits into from
Feb 20, 2024

Conversation

dimshitc
Copy link
Collaborator

Changed value low for days supply to 1 and value high for refills to 24

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1c48fcb) 86.48% compared to head (bb2c6df) 87.96%.
Report is 9 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #528      +/-   ##
===========================================
+ Coverage    86.48%   87.96%   +1.47%     
===========================================
  Files           16       16              
  Lines          888      939      +51     
===========================================
+ Hits           768      826      +58     
+ Misses         120      113       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@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.

In short: I suggest to also use a plausibleValueLow of 1 for drug_exposure.quantity, drug_exposure.refills, in line with the days_supply change made here.

Looks good, but I would like to suggest to apply the same logic of 'not negative or 0' to the other plausibleValueLow. In attached screenshot I listed all plausibleValueLow (v5.4) that are 0 or 1. Interestingly, the drug_exposure.quantity is the only quantity field that is allowed to be 0. Note that dose_era.dose_value is a float and can technically be any some very small fraction and drug_era.gap_days will be 0 if no 'drug free days' are observed.
image

@dimshitc
Copy link
Collaborator Author

dimshitc commented Feb 15, 2024

drug_exposure.quantity can be less than 1. if it's 1/2 of tablet, for example. or if the drug concept is an ingredient, then it's the amount of this substance, which can be between 0 and 1 as well.
in theory this can be applied to the Device.quantity when applicable to the blood products or contrast, but since there's no use cases, I would keep it as plausibleValueLow= 1.

I agree with refills change though, applied it in the latest commit

@katy-sadowski
Copy link
Collaborator

refills can be 0 though right? for a one-time rx?

@dimshitc
Copy link
Collaborator Author

I think for one time you just leave refills as NULL

@katy-sadowski
Copy link
Collaborator

I think for one time you just leave refills as NULL

hmm, i don't see that as a convention in CDM, though ... if it won't cause any issue to have refills as 0 i think we should allow it.

@dimshitc
Copy link
Collaborator Author

yep, probably better leave it as 0. in our data there are a lot of these refills = 0 as well.
I also noticed that quantity is 0 as well in some rows which is obviously an error which was overlooked by the DQD, so we might put drug_exposure.quantity valueLow = 0.0000001 (some very small value), or change the SQL by putting <= instead of <.
Personally I like 0.0000001 more, since it doesn't require the review of all values after operator change

@katy-sadowski
Copy link
Collaborator

I also noticed that quantity is 0 as well in some rows which is obviously an error which was overlooked by the DQD, so we might put drug_exposure.quantity valueLow = 0.0000001 (some very small value), or change the SQL by putting <= instead of <. Personally I like 0.0000001 more, since it doesn't require the review of all values after operator change

Ha, interesting. I think .0000001 makes sense. If we change the operator to <=, then we will have the same issue with plausibleValueLow = 1. Because then all the legitimate 1's would fail and we'd need to put 0.999999.

@dimshitc dimshitc merged commit c065c84 into develop Feb 20, 2024
10 checks passed
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.

3 participants