-
-
Notifications
You must be signed in to change notification settings - Fork 459
Persistence extensions: preserve item unit in results #4951
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
base: main
Are you sure you want to change the base?
Conversation
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/javascript-persistance-totals-off/165099/19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates persistence extensions to preserve the item unit in calculation results instead of converting to system units. Previously, methods like riemannSum, variance, and deviation were converting quantities to system units (e.g., Kelvin for temperatures) before calculations, which resulted in outputs in system units rather than the original item units.
Key changes include:
- Modified variance and deviation calculations to use item units instead of system units
- Updated Riemann sum calculations to preserve item units in results
- Removed Kelvin offset handling from test utilities
- Added comprehensive test coverage for EnergyPrice quantities
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| TestPersistenceService.java | Removes Kelvin offset handling and simplifies test value generation |
| PersistenceExtensionsTest.java | Updates test expectations to verify item units are preserved and adds EnergyPrice test coverage |
| PersistenceExtensions.java | Modifies core calculation logic to use item units instead of system units and adds documentation notes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...persistence/src/main/java/org/openhab/core/persistence/extensions/PersistenceExtensions.java
Outdated
Show resolved
Hide resolved
...persistence/src/main/java/org/openhab/core/persistence/extensions/PersistenceExtensions.java
Show resolved
Hide resolved
...persistence/src/main/java/org/openhab/core/persistence/extensions/PersistenceExtensions.java
Show resolved
Hide resolved
...persistence/src/main/java/org/openhab/core/persistence/extensions/PersistenceExtensions.java
Show resolved
Hide resolved
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/persistence-in-rule/166016/3 |
d9f39f5 to
602e21a
Compare
602e21a to
a61a111
Compare
a61a111 to
3cfd5b4
Compare
|
@wborn Did you review this PR already? |
|
Haven't looked at the code yet @kaikreuzer. I only requested some initial @copilot feedback. |
Signed-off-by: Mark Herwege <[email protected]>
3cfd5b4 to
00d06ed
Compare
|
Rebased |
See discussion: https://community.openhab.org/t/javascript-persistance-totals-off/165099
Many of the persistence extensions are internally converting to the system unit to make them indepent of unit offsets. The Quantity results returned where in this system unit, and not in the item unit. Typically, persisted values are stored in the item unit.
This PR tries to return the item unit (or a unit derived from it) as much as possible.
sumSince,sumUntilandsumBetweenwill now always return the result in the item unit.riemannSumSince,riemannSumUntilriemannSumBetweenwill return the result in item unit *s.sumSince,sumUntilandsumBetweenwill now always return the result in the item unit.varianceSince,varianceUntilandvarianceBetweenreturn the result in item unit ^2.deviationSince,deviationUntilanddeviationBetweenreturn the result in item unit.Depending on the item unit, the resulting unit may get simplified automatically. This does not happen for pre-defined composed units (such as
kWh/swill not automatically eliminate the time dimension, butkW * h / swill).The changes in this PR are changing the results of the
riemannSum,variance, anddeviationextensions for Temperatures as they will now be calculated using the item unit, and not the system unit for temperatures (K). I thought long about this, but I believe most people will expect these calculations not to be done inK. Note thatevolutionRatewas already doing the calculation in the item unit, making the persistence extension inconsistent.