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

Better y-axis titles when showing multiple sensors #1346

Merged
merged 7 commits into from
Mar 5, 2025

Conversation

Flix6x
Copy link
Contributor

@Flix6x Flix6x commented Mar 2, 2025

Description

  • Add support for more shared base units in y-axis titles
  • Add support for shared price units, currencies, speeds and percentages
  • Added changelog item in documentation/changelog.rst

Look & Feel

Before:

image

After (notice the y-axis title changed from Value to Temperature):

image

How to test

The docstrings of the new unit util functions include doctests. Doctests aren't part of the automated test suite, though, but they will be - #1347

@Flix6x Flix6x added UI Units Deals with unit conversion labels Mar 2, 2025
@Flix6x Flix6x added this to the 0.25.0 milestone Mar 2, 2025
@Flix6x Flix6x self-assigned this Mar 2, 2025
@Flix6x Flix6x requested a review from BelhsanHmida March 2, 2025 22:00
* docs: add doctest example for price unit

Signed-off-by: F.N. Claessen <[email protected]>

* docs: changelog entry

Signed-off-by: F.N. Claessen <[email protected]>

* feat(ci): run all doctests in the utils subpackage

Signed-off-by: F.N. Claessen <[email protected]>

* dev: skip other tests (for testing the new feat)

Signed-off-by: F.N. Claessen <[email protected]>

* fix: fix all failing doctests

Signed-off-by: F.N. Claessen <[email protected]>

* Revert "dev: skip other tests (for testing the new feat)"

This reverts commit 5bedeca.

* docs: changelog entry

Signed-off-by: F.N. Claessen <[email protected]>

* refactor: shorter doctest

Signed-off-by: F.N. Claessen <[email protected]>

* feat: add doctests for data/schemas to ci

Signed-off-by: F.N. Claessen <[email protected]>

* fix: doctest syntax

Signed-off-by: F.N. Claessen <[email protected]>

* fix: do not repeat running normal tests

Signed-off-by: F.N. Claessen <[email protected]>

---------

Signed-off-by: F.N. Claessen <[email protected]>
@Flix6x
Copy link
Contributor Author

Flix6x commented Mar 3, 2025

@BelhsanHmida This is the important bit to check. The rest is of this PR comes from me merging #1347 into this PR branch, sorry.

@BelhsanHmida
Copy link
Contributor

I looked into the relevant refactoring, and it looks good! I tested it, and the results are as expected. I also tested the code overall, and it works fine. However, I found cases where shared units don’t work properly (e.g., wind speed in km/h and m/h). I’m guessing these units aren’t implemented yet, but I just wanted to note this.

Here are the test results for a couple of units: Screencast from 2025-03-05 01-51-43.webm

@BelhsanHmida
Copy link
Contributor

BelhsanHmida commented Mar 5, 2025

Another feature I wanted to ask about is handling sensors with the same dimension but different units—for example, one in MW and another in kW. Currently when we overlay these sensors, the Y-axis shows Value (a.u.). Should we consider standardizing the Y-axis to the smallest unit (e.g., kW in this case) for better clarity. so it would be Power (kW)?

Screenshot from 2025-03-05 02-02-22

@Flix6x
Copy link
Contributor Author

Flix6x commented Mar 5, 2025

High-quality review, thank you!!

I found cases where shared units don’t work properly (e.g., wind speed in km/h and m/h). I’m guessing these units aren’t implemented yet, but I just wanted to note this.

Good finds. For now I'll either include xfail test cases for EUR (currency) and km/h (speed) units, or implement fixes for some of these cases already.

handling sensors with the same dimension but different units—for example, one in MW and another in kW. Currently when we overlay these sensors, the Y-axis shows Value (a.u.). Should we consider standardizing the Y-axis to the smallest unit (e.g., kW in this case) for better clarity.

I don't think we should unless we also scale the MW data to kW. I'll open a new issue for this, because it would be a good feature imo. #1352

…ts/shared-base-units

# Conflicts:
#	documentation/changelog.rst
@Flix6x Flix6x merged commit bf49530 into main Mar 5, 2025
8 of 10 checks passed
@Flix6x Flix6x deleted the feat/units/shared-base-units branch March 5, 2025 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI Units Deals with unit conversion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants