Introduce state-based patient model #614
Conversation
ClFeSc
added
enhancement
* Continuation of #490 Co-authored-by: Florian <hpistudent72@users.noreply.github.com>
ClFeSc
force-pushed
the
feature/335-402-patient-model
branch
from
79d5fa2
to
66431cd
Compare
shared/src/store/action-reducers/utils/calculate-treatments.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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.
| patient, | ||
| state.configuration.pretriageEnabled, | ||
| state.configuration.bluePatientsEnabled | ||
| ) !== 'black' && !Patient.isInVehicle(patient) |
There was a problem hiding this comment.
use isOnMap(patient) instead
There was a problem hiding this comment.
And even Patients that are in Vehicles should experience State Changes, but this is maybe out of scope for this
| @IsOptional() | ||
| @IsNumber() | ||
| @Min(0) | ||
| public readonly latestTime?: number; |
There was a problem hiding this comment.
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.
| 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 | ||
| ); | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
I feel like this needs a Comment
| export function parsePatientData(importString: string) { | ||
| const patientData: PatientData[] = []; | ||
|
|
||
| const splitString = importString.split('|'); |
There was a problem hiding this comment.
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
| healthStateInformation: string[], | ||
| pretriageInformation: PretriageInformation | ||
| ) { | ||
| const phaseTime = 12 * 60 * 1000; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Just because I saw this now: that's 12 minutes (min * s * ms).
| 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++) { |
|
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. |
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. |
|
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. |
|
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. |
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.