`SimpleBattery` is sensitive to floating point imprecision
I was using randomly assigned values for initializing a SimpleBattery, and I noticed the update method sporadically failed at this line:
https://github.com/dos-group/vessim/blob/e7fe48ecbf68c3c6b05e437ef96090fa7db8189c/vessim/storage.py#L57
This is despite the initialization being valid, and the supplied values for capacity, charge level and min_soc being validated by me beforehand. I did not update min_soc during runtime. I suspected the error might be because of floating point loss, and by inserting some print statements before the assertion, I noticed it seems I was right:
step: 0, min soc: 0.5881180156958709, soc: 0.7874107655567011, capacity: 54.79224064535798, charge level: 43.14400015312832
step: 1, min soc: 0.5881180156958709, soc: 0.5881180156958707, capacity: 54.79224064535798, charge level: 32.22430384387857
Traceback (most recent call last):
# [...]
AssertionError: Minimum SoC can not be smaller than the current SoC
The first time the battery is updated everything is fine, but the battery will be fully discharged. On the next call then, because of floating point imprecision, the charge level was set correctly, but the calculated soc is slightly below min_soc.
It's not a pressing issue for me, but in general, it surely would be nice if the simulation did not crash despite the user-provided parameters for the battery being valid.
- One could use the built-in
decimalmodule to perform exact arithmetic operations. - Alternatively, the assertion could be rewritten using
math.isclose. That way floats will suffice - it might be worth thinking whether this is needed in any of the other comparisons in theupdatecode.
I assume the purpose of that assertion is to check against invalid changes made to min_soc during runtime / between updates? (The update method itself should always leave the battery in a valid state :relaxed:)
Another option then would be to remove the assertion from the update code, and transform min_soc into a property with a proper setter that performs this check and throws a ValueException if necessary. It would then also be clearer exactly when an invalid set to min_soc happened - not just when the next update step is performed.
Actually, I'd argue in favor of making min_soc a settable-property even if another solution is applied.