-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fixes: CDR engine fixes for duplicate use of actions across rules in addition to an optimization #7268
Conversation
hi @hanksterr7 , Interestingly this fix/optimization amplifies that filter bug (where iterating through the filter on all the dates rather than just the target date), which you have fixed in your PR. Because of this, I think it may be best to get your PR into the codebase before this PR. |
@bradymiller : Interesting. Ok, I've reviewed the first part of what you've changed. Previously, in lines 1037 to 1041, $reminder_due would be set to the status the rule will have if its target does NOT pass for the current value of $dateFocus You have now changed $reminder_due to be the status the rule WILL have if its target DOES pass for the current $dateFocus. Ok. that is cleaner. Was very confusing previously. Note that passing during iteration #1 means the rule status is "not due". If it does not pass during iteration #1 but passes during iteration #2, its status is "due soon". If it does not pass until iteration #3, its status is "due". And if it does not pass during any of iterations #1, #2 or #3, the status is "past due". Note also that the status is "past due" if EITHER the target data was found, but its date was prior to today - target interval - past due interval, OR the target data was not found at all. There does not seem to be any effort to record a distinction between these two possibilities In lines 1068 through 1106, you have adjusted the logic to match the new value of $reminder_due that is set for each $dateFocus value (i.e. for reach pass through the loop of $target_dates values) I don't understand the meaning of the different possible values of $mode. If you could add some comments to the code explaining what these values are, and how and why the code should behave differently for each of them, that would help. In particular, explaining what the difference is between $mode = "reminders-all" vs "report", vs any other values $mode could have. -- edit: I see you do describe these $mode values in your re-working of reminder_results_integrate(). Perhaps adjusting your comments in test_rules_clinic() to alert the reader to review the code/comments in reminder_results_integrate() for the meaining of $mode would help :) Lines 1077 - 1080 determine if reminder_results_integrate() should get called if the target passes for the current value of $dateFocus. LInes 1095 and 1096 determine if reminder_results_integrate() should get called when the target does NOT pass for the current value of $dateFocus I'm going to describe the behavior that I believe the code is executing. Please confirm this is what you want. The code behaves differently depending on whether or not $mode is "reminders-all". I will describe each option separately First, when $mode is "reminders-all": If the target passes during pass #1 ($reminder_due = "not_due"), reminder_results_integrate() gets called in line 1089 with due_status of "not_due". Further processing of $target_dates ceases, due to the "break" in 1093. If it doesn't pass during pass #1, reminder_results_integrate() does not get called in either lines 1089 or 1103, and pass #2 begins If the target passes during pass #2 ($reminder_due = "soon_due"), reminder_results_integrate() gets called in line 1089 with due_status of "soon_due". Further processing of $target_dates ceases due to the "break" in 1093. If it does not pass during pass #2, again reminder_results_integrate() does not get called in either lines 1089 or 1103, and pass #3 begins If the target passes during pass #3 ($reminder_due = "due"), reminder_results_ingrate() gets called in line 1089 with due_status of "due". Further processing of $target_dates ceases due to the "break" in 1093. If it does not pass during pass #3, reminder_results_integrate() now gets called in line 1103 and sets due_status to "past_due" The above behavior makes sense Now, if $mode is not "report" AND $mode is not "reminders-all": If target passes during pass #1: reminder_results_integrate() does NOT get called in either 1089 or 1103, and processing stops. You have the comment: "note this is skipped if not_due and not in reminders-all mode", so the behavior is consistent with this comment. It would help if your comment would explain WHY you do not want to call reminder_results_integrate() in this case. I don't understand why you want this behavior, but I also don't understand what it means for $mode to not be "report" and not be "reminders-all" If target does not pass during pass #1 but passes during pass #2, reminder_results_integrate() is called in 1089 and with due_status of "soon_due" and processing stops If target does not pass during passes #1 or #2 but passes during pass #3, reminder_results_integrate() is called in 1089 and with due_status of "due" and processing stops If target does not pass during passes #1, #2 or #3, reminder_results_integrate() gets called in line 1103 and sets due_status to "past_due" Given your comment of "note this is skipped if not_due and not in reminders-all mode", I guess the behavior for this $mode choice is also consistent with your desires, so I guess the code is good to go But I also agree it would be good if this were merged with my code that moves the filter processing outside the looping through $target_dates values I will now review your changes to reminder_results_integrate(). I won't have thoughts on that until tomorrow or Tuesday Thanks |
hi @hanksterr7 ,
|
@@ -2777,30 +2781,6 @@ function collect_database_label($label, $table) | |||
return $returnedLabel; | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to help with the review on this, I just removed this function since it was not being used.
ce8ccf5
to
0759cd5
Compare
hi @hanksterr7 . |
Thanks Brady. Sorry for being off the grid on this for a while. I'll be done with work projects soon and can dive back in on this-- Hank
On Sunday, March 24, 2024 at 05:30:35 AM EDT, Brady Miller ***@***.***> wrote:
hi @hanksterr7 .
I incorporated your filter fix in this PR, so this code would work correctly. Gave you authorship to the commit that has the filter fix code, which i followed with another commit that had some minor changes in your code. So far testing well. Plan to repeat extensive testing before bringing this in. Let me know if you have thoughts/issues.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Hi @bradymiller |
0759cd5
to
6b2ddce
Compare
@hanksterr7 , |
hi @hanksterr7 , |
Hi @bradymiller |
@stephenwaite, if you can, try creating some complicated rules that have multiple filters and targets, where the different filters and targets are of different category types (e.g. age, vs vitals, vs diagnoses, vs social hx, vs appointments, vs meds, etc), and where you have a mix of inclusion and exclusion filters/targets, and where the dates of data the targets are looking at varies relative to the due soon/due/past due intervals (such as having one target, such a diagnosis, entered a week ago, and a blood pressure entered one year less a week ago, for a target interval of 1 year, and a warning interval of one month, and a past due interval of 2 months. That scenario is what my changes tried to make work better. Compare behavior of that scenario in current release vs what this PR is doing. Thanks |
18020d2
to
5b0305d
Compare
Hi @bradymiller and @stephenwaite To help you do some testing of of the changes in my code, I offer what is below These will show how behavior changes when using the code in this PR compared to the code in the main branch. Note that the main branch has already been patched to cause the required/optional labels in the Rules UI to now be correct This could be helpful to include in release notes Preparation: -- create Rule #2, with configuration: -- create a patient with age: 25, gender: female, Medical Problem: ICD10 diagnosis: Chronic Obstructive Pulmonary Disease, J44.9 (or any problem of your choosing), begin date; blank, end date: blank (modify date will be set to the date you created the problem) issues being tested/fixed: -- Issue #2: optional filter failure in one category doesn't allow checking optional or required filter in next category -- Issue #3: rule gets status of "due" instead of "past due" if filter data has date between T-PastDueInterval and T. -- Issue #4: if have two required targets, one with data date between T-PastDueInterval and T, and the other in "due" range, rule shows as past due instead of due, since on no $dateFocus pass do both targets succeed. -- Issue #5: if have more than one target in a target/action group, and if optional ones in a category do not succeed, subsequent categories are not checked. Reasons are same as for Issue #2. |
5b0305d
to
b9c3af6
Compare
fyi, getting close on this. Just a bit more testing (guessing will get into codebase over the weekend) |
setup an up for grabs demo |
@stephenwaite : what is the "up for grabs demo"? Is my code included in it? Is there anything I need to do to help make this happen? |
@stephenwaite and @bradymiller The alpha system behaves as I would expect, based on my fixes |
…addition to an optimization The optimization both clarifies the code regarding which $reminder_due status is chosen and also minimizes the calls to reminder_results_integrate, which could get resource intensive if overuse it.
- improved handling of filters/targets for inclusion/exclusion flags, and where multiple filters/targets exist across classes of patient data - fixing right side of target interval used for target testing at value of $dateTarget passed to test_rules_clinic() instead of a value that floats with changes to left side of the interval - fix to divide by zero error in evaluating percent of passing rules more details at: openemr#7153 openemr#7157 openemr#7161
b9c3af6
to
5be871e
Compare
hi @hanksterr7 , |
way to go @hanksterr7 @bradymiller !!! |
fixes: CDR engine fixes (cherry picked from commit e122860)
This reverts commit 3a33b4e.
Great to see all my changes now in the master branch! Thanks everyone for all the effort and support |
fixes #7266
Fixes #7153
Fixes #7157
Fixes #7161
CDR engine fixes for duplicate use of actions across rules in addition to an optimization
The optimization both clarifies the code regarding which $reminder_due status is chosen and also minimizes the calls to reminder_results_integrate, which could get resource intensive if overuse it.
Also brought in fixes from #7160