Introduce state-based patient model #614

ClFeSc · 2023-01-23T10:04:19Z

Closes #335
Closes #402

Continuation of #490. Note that the remarks and comments made there were not incorporated here, so they're still relevant.

Design by @hpistudent72

@hpi-sam/bp2022hg1 You can assign reviewers at your own discretion.

* Continuation of #490 Co-authored-by: Florian <[email protected]>

shared/src/store/action-reducers/utils/calculate-treatments.spec.ts

shared/src/utils/validators/is-id-map.ts

benn02

A few general Things: It would be nice if there were more than 1 Test Patient available w/o an external CSV. Not only for testing, but also it would be nice if basic usability was demonstrable w/o said external file. I would suggest a build in CSV that then must also adhere to our new licence(keep in mind). I would also suggest that the Speed in which Patients switch between phases is communicated more clearly to the user. In the offline equivalent, the trainers do also control the time, not a transparent factor. I would suggest changing the UI in a way where the trainers can control the time between phase changes directly. Lastly, migrations are missing. Since there is no direct translation between old and new patients, maybe deleting all patients would be an option. but keep in mind to keep statistics so exercises done before this change can be evaluated with the software after this change.

benn02 · 2023-02-01T14:43:27Z

backend/src/exercise/patient-ticking.ts

+ patient,
+ state.configuration.pretriageEnabled,
+ state.configuration.bluePatientsEnabled
+ ) !== 'black' && !Patient.isInVehicle(patient)


use isOnMap(patient) instead

And even Patients that are in Vehicles should experience State Changes, but this is maybe out of scope for this

benn02 · 2023-02-01T16:43:31Z

shared/src/models/patient-health-state.ts

 @IsOptional()
 @IsNumber()
+ @Min(0)
 public readonly latestTime?: number;


I don't really understand the need for two variables with not so clear names. Earliest Time What And why do we need two Variables to display how long the patient is in the current state, isn't one enough? This was not changed, but I feel like this could be made clearer maybe in this PR.

benn02 · 2023-02-01T16:49:36Z

shared/src/models/patient-template.ts

+ const randomizedNextStateConditions = Object.values(
+ state.nextStateConditions
+ ).map((condition) => {
+ let newEarliestTime = condition.earliestTime;
+ if (newEarliestTime) {
+ newEarliestTime = randomizeValue(
+ newEarliestTime,
+ 0.2
+ );
+ }
+ let newLatestTime = condition.latestTime;
+ if (newLatestTime) {
+ newLatestTime = randomizeValue(newLatestTime, 0.2);
+ }
+ return ConditionParameters.create(
+ newEarliestTime,
+ newLatestTime,
+ condition.isBeingTreated,
+ condition.requiredMaterialAmount,
+ condition.requiredNotArztAmount,
+ condition.requiredNotSanAmount,
+ condition.requiredRettSanAmount,
+ condition.requiredSanAmount,
+ condition.matchingHealthStateName
+ );
+ });


I don't really understand why there needs to be randomization involved apart from Names. I think it shouldn't be the software that chooses specific details about the patient that influences the optimal behaviour of participants. As a trainer, I would want full control. If I want randomization, I could either randomize the CSV Data or I would like to choose what is randomized and what not.

The idea was that not every patient should be identical, meaning they would be easier to predict. With a little randomness, it's possible to generate a more natural behavior of patients without blowing up the CSV file.

benn02 · 2023-02-01T16:56:38Z

shared/src/store/action-reducers/exercise.ts

+ if (currentPatient.treatmentHistory.length === 0) {
+ for (let i = 0; i <= (60 * 1000) / tickInterval; i++) {
+ currentPatient.treatmentHistory.push({
+ gf: 0,
+ material: 0,
+ notarzt: 0,
+ notSan: 0,
+ rettSan: 0,
+ san: 0,
+ });
+ }
+ }


I feel like this needs a Comment

benn02 · 2023-02-01T17:05:36Z

frontend/src/app/pages/exercises/exercise/shared/utility/parse-csv.ts

+export function parsePatientData(importString: string) {
+ const patientData: PatientData[] = [];
+
+ const splitString = importString.split('|');


This applies to the one CSV File that we got I would like to discuss whether we should support more generic CSVs but most likely not in this PR

benn02 · 2023-02-01T17:07:33Z

frontend/src/app/pages/exercises/exercise/shared/utility/parse-csv.ts

+ healthStateInformation: string[],
+ pretriageInformation: PretriageInformation
+) {
+ const phaseTime = 12 * 60 * 1000;


I have seen this or similar constants (x * 60 *1000)as throughout all the Changes, and I don't really understand them. Maybe I would put them in a combined place.

Just because I saw this now: that's 12 minutes (min * s * ms).

benn02 · 2023-02-01T17:12:02Z

shared/src/store/action-reducers/exercise.ts

 currentPatient.stateTime = patientUpdate.nextStateTime;
 currentPatient.treatmentTime = patientUpdate.treatmentTime;
- currentPatient.realStatus = getStatus(currentPatient.health);
+ if (currentPatient.treatmentHistory.length === 0) {
+ for (let i = 0; i <= (60 * 1000) / tickInterval; i++) {


Magic Number

christianzoellner · 2023-03-26T08:45:36Z

Is this PR still ongoing?

ClFeSc · 2023-03-26T16:52:58Z

Is this PR still ongoing?

Theoretically, yes. Practically, I haven't had enough time over the course of the semester and time with the exams to work on this. Also, I have had some problems understanding the code as I have not written it. But I'll try to look into this again in the next few weeks.

Dassderdie · 2023-03-26T17:18:07Z

Theoretically, yes. Practically, I haven't had enough time over the course of the semester and time with the exams to work on this. Also, I have had some problems understanding the code as I have not written it. But I'll try to look into this again in the next few weeks.

Was there already a consensus about how to handle migrations of patients here? While I believe it is in theory possible to migrate them, this would be a lot of work.

In addition, I believe the code as @hpistudent72 wrote it would be significantly less performant than now (I estimate ca. one order of magnitude) . I proposed a solution to not only fix this, but also improve the performance by ca. one order of magnitude and reduce the size of history exports. Implementing this would also require even more work.

TLDR: this PR could be more work than just a simple merge.

christianzoellner · 2023-03-26T17:36:04Z

Thanks @ClFeSc and @Dassderdie for your responses and your continued activities as a volunteer developers on this project!

I really do not want to push you by raising this issue again, but the topic of the patient model came up during a meeting I had this weekend at BABZ.

Sadly I am not really familiar with the code, but I am available to answer any questions you might have about the underlying model and concept.

Dassderdie · 2023-03-26T22:24:22Z

Maybe it would also help if this PR would be split up into multiple smaller ones. For example, the refactored patient system could be separated from the CSV import and the smaller UI improvements.

ClFeSc added enhancement New feature or request shared Issues mainly related to shared Legacy Issues that were created by the 2021 BP labels Jan 23, 2023

ClFeSc requested a review from Dassderdie January 23, 2023 10:04

ClFeSc self-assigned this Jan 23, 2023

Introduce state-based patient model

66431cd

* Continuation of #490 Co-authored-by: Florian <[email protected]>

ClFeSc force-pushed the feature/335-402-patient-model branch from 79d5fa2 to 66431cd Compare January 23, 2023 10:09

ClFeSc mentioned this pull request Jan 23, 2023

Patient model (from bacherlor thesis) #490

Closed

ClFeSc commented Jan 23, 2023

View reviewed changes

shared/src/store/action-reducers/utils/calculate-treatments.spec.ts Outdated Show resolved Hide resolved

Dassderdie reviewed Jan 23, 2023

View reviewed changes

shared/src/utils/validators/is-id-map.ts Outdated Show resolved Hide resolved

ClFeSc added 2 commits January 23, 2023 14:49

Remove unused parameter in test

ab02563

Remove old code

a38df9b

benn02 self-requested a review February 1, 2023 10:09

benn02 reviewed Feb 1, 2023

View reviewed changes

anonym-HPI mentioned this pull request Feb 3, 2023

add AGPL v3.0 (or later) license #636

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce state-based patient model #614

Introduce state-based patient model #614

ClFeSc commented Jan 23, 2023

benn02 left a comment

benn02 Feb 1, 2023

benn02 Feb 1, 2023

benn02 Feb 1, 2023

benn02 Feb 1, 2023

ClFeSc Feb 9, 2023

benn02 Feb 1, 2023

benn02 Feb 1, 2023

benn02 Feb 1, 2023

Dassderdie Mar 26, 2023

benn02 Feb 1, 2023

christianzoellner commented Mar 26, 2023

ClFeSc commented Mar 26, 2023

Dassderdie commented Mar 26, 2023

christianzoellner commented Mar 26, 2023

Dassderdie commented Mar 26, 2023

Introduce state-based patient model #614

Are you sure you want to change the base?

Introduce state-based patient model #614

Conversation

ClFeSc commented Jan 23, 2023

benn02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianzoellner commented Mar 26, 2023

ClFeSc commented Mar 26, 2023

Dassderdie commented Mar 26, 2023

christianzoellner commented Mar 26, 2023

Dassderdie commented Mar 26, 2023