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

SimpleBattery is sensitive to floating point imprecision #204

Open
Impelon opened this issue Apr 28, 2024 · 1 comment
Open

SimpleBattery is sensitive to floating point imprecision #204

Impelon opened this issue Apr 28, 2024 · 1 comment

Comments

@Impelon
Copy link
Contributor

Impelon commented Apr 28, 2024

I was using randomly assigned values for initializing a SimpleBattery, and I noticed the update method sporadically failed at this line:

assert self.min_soc <= self.soc(), "Minimum SoC can not be smaller than the current SoC"

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 decimal module 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 the update code.
@Impelon
Copy link
Contributor Author

Impelon commented Apr 29, 2024

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 ☺️)

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.

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

No branches or pull requests

1 participant