search

ErichBSchulz / lung

Lung and Ventilation Models
3 stars 2 forks source link

Feedback on the `step` function #1

Open dmitriz opened 4 years ago

dmitriz commented 4 years ago

Documenting my comments from today's conversation for future reference on the step function:

The current signature is 

def step(config, state, p)

This requires preparing objects with correct props each time the function is used or tested. Also it mutates the state making tests more fragile - what if someone forgets to clear the state between tests - all tests may continue working without doing the right job that may lead to the false assumption that tests are working correctly without any way to catch these errors.

Another concern - you cannot write tests based on function signature, you need to carefully look over all object props appearing inside the code and test for each one of them and the choice of names and their types is now coupled to all functions and all tests.

Actual values used by step function:

// props of config
alphaA, alphaN, alphaR, alphaS, min_breath_envelope_delta, sample_frequency
// props of state
p, Tpeak, Thigh, Vhigh, vhigh, Vlow, vlow, vhigh-state.vlow, inhaling, PEEP, RIP, RR
// other args
p

Mutation can be avoided by making this function return the dictionary of new values and subsequently merge it into the state.

RobertLRead commented 4 years ago

I agree; mutation is a bad idea unless forced into it. It is better to remain in the world of "pure functions" which are side-effect free. In this case, step could take a state and return a state, to avoid mutation state.

I'm not entirely sure the concept of a "ventilator state" is useful, although I can understand Erich's approach of thinking of the main VentOS control algorithm as something that is managing state. We probably do need a concept of "time"---that is, we need to know how many milliseconds we are into a particular breath cycle for a control mode.

Note that the test framework may very need state, as apart from the control mode. That is, in running a test, the context of the test defines a state---is that pressure sensor reading high or low? However, that need not be a part of the control mode algorithm, which would be better treated as a pure function.