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

Patient model (from bacherlor thesis) #490

Closed
wants to merge 9 commits into from

Conversation

hpistudent72
Copy link
Contributor

@hpistudent72 hpistudent72 commented Jul 25, 2022

Closes #335
Closes #402

Copy link
Contributor

@ClFeSc ClFeSc left a comment

Choose a reason for hiding this comment

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

I haven't tested it out yet.

backend/src/exercise/patient-ticking.ts Show resolved Hide resolved
backend/src/exercise/patient-ticking.ts Show resolved Hide resolved
@@ -98,7 +102,36 @@ <h5 class="popover-header">
Verletzungs-Geschwindigkeit:
</th>
<td class="font-monospace">
&times;{{ patient.timeSpeed }}
<select
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a dropdown here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would mean writing code, just to end up with a self implemented select input.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Dassderdie What do you think?

shared/src/models/patient-template.ts Outdated Show resolved Hide resolved
shared/src/models/patient.ts Outdated Show resolved Hide resolved
shared/src/models/patient.ts Outdated Show resolved Hide resolved
shared/src/store/action-reducers/exercise.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ClFeSc ClFeSc left a comment

Choose a reason for hiding this comment

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

I still haven't tried it out, but the code looks much better now.

Copy link
Contributor

@ClFeSc ClFeSc left a comment

Choose a reason for hiding this comment

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

Please fix these visual bugs (especially the one with the colors), then I can review this further.

return 'black';
case 'blau':
return 'blue';
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe throw something here? Or use as 'blau' | 'gelb' | 'grün' | 'rot' | 'schwarz' in the switch (color[0]) line?
But don't have an empty default and then just return something you know will never match. Probably as is the better solution. @Dassderdie Any thoughts on this?

Copy link
Collaborator

@Dassderdie Dassderdie Aug 1, 2022

Choose a reason for hiding this comment

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

Instead of a switch statement you should use an object literal as a map.
This should then result in something like:

colorTranslationMap[color[0]]!;

) {}
}

function generateHealthStates(
Copy link
Contributor

Choose a reason for hiding this comment

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

Something about the generation of the condition parameters seems to be wrong, only two of the templates of the CSV actually have conditions. For this reason, the patients don't change their state.

@Dassderdie Dassderdie mentioned this pull request Aug 23, 2022
6 tasks
@anonym-HPI
Copy link
Contributor

@hpistudent72 Are you planning to finish this or should someone else continue this PR?

@anonym-HPI
Copy link
Contributor

@Dassderdie @ClFeSc One of you interested in continuing this? I could also try to get this done.

@ClFeSc
Copy link
Contributor

ClFeSc commented Oct 5, 2022

I could try to do it.

@ClFeSc ClFeSc self-assigned this Oct 6, 2022
@ClFeSc ClFeSc marked this pull request as draft October 6, 2022 18:06
@anonym-HPI
Copy link
Contributor

I could try to do it.

Ok, if you need help, just write.

@lukasrad02
Copy link
Contributor

@ClFeSc are you still working on this? This feature would be interesting for our future development.

We could also try to support you on this, this way we could also get familiar with the new features quickly.

Comment on lines +98 to +101
currentPatient.treatmentHistory.shift();
currentPatient.treatmentHistory.push(
patientUpdate.newTreatment
);
Copy link
Collaborator

@Dassderdie Dassderdie Jan 18, 2023

Choose a reason for hiding this comment

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

This is probably a big performance problem as this means that for every tick for every patient, a new array with 60 items is created (immutability via the immer.js proxy).

Here is a proposal for a better solution:

This value probably doesn't change very often. Therefore, one could compress intervals with the same treatment status.

interface Interval {
    /**
     * Corresponds to the exerciseTime
     */
    startTime: number,
    value: { gf: number, .... }
}

class TreatmentHistory {
    // in seconds
    static readonly maxLength = 60;

    // If the newTreatment is different to the treatment in the last interval, add it (with the current exerciseTime) 
    // and remove all intervals that are older than (exerciseTime - maxLength)
    // else do nothing
    static addTreatment(intervalls: Intervall[], newTreatment: {gf: number, ...}, exerciseTime)

    static getAverageTreatment(...)
}

A Patient would then have treatmentHistory: Intervall[] or something like this.

This would reduce write time (expensive due to immutability) and slightly increase read time (not done very often).

One could also either remove the PatientUpdates from the Tick action or at least only send those for patients that changed their treatment. This would reduce the state-history size by a lot and could either slightly reduce or increase the performance (more calculation on the client vs. less parsing, reduced usage of the WebSocket connection, and less overhead on the server). This would also have big repercussions on the migrations.

Guidelines: "Premature optimization is the root of all evil.", Use the benchmark script.

@ClFeSc
Copy link
Contributor

ClFeSc commented Jan 19, 2023

@ClFeSc are you still working on this? This feature would be interesting for our future development.

We could also try to support you on this, this way we could also get familiar with the new features quickly.

Generally, yes. However, currently, I'm a bit short of time. But I'll look into resolving the merge conflicts this weekend, and we can see where we'll go from there.

ClFeSc added a commit that referenced this pull request Jan 23, 2023
Co-authored-by: Florian <[email protected]>

* Continuation of #490
ClFeSc added a commit that referenced this pull request Jan 23, 2023
* Continuation of #490

Co-authored-by: Florian <[email protected]>
ClFeSc added a commit that referenced this pull request Jan 23, 2023
* Continuation of #490

Co-authored-by: Florian <[email protected]>
@ClFeSc
Copy link
Contributor

ClFeSc commented Jan 23, 2023

There is now #614 that further tracks the progress of this. Note that the remarks and comments made here are currently still relevant. Any further remarks and comments are added to #614, please.

@ClFeSc ClFeSc closed this Jan 23, 2023
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.

Change Patientsystem from life-point-based to state-based Allow changing the treatment factor
5 participants