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

feat: add scale_factor and extend value_factor (next) #877

Closed
wants to merge 12 commits into from

Conversation

X-Ryl669
Copy link

@X-Ryl669 X-Ryl669 commented Dec 1, 2022

I've bootstrapped from PR #608 that was stalling.

I've fixed the remarks and fixed the implementation to also work with negative scale factor.
This allow to have a negative value_multiplier (for example, when injecting current in the grid) per entity and also a value_factor per entity.

I've also fixed the documentation.

This gives this:
image

where the blue graph is a negative valued since it's a production not a consumption:
image

bdraco and others added 4 commits May 12, 2022 13:32
* Override icon with an image URL

added image attribute to override icon with a URL.

(cherry picked from commit dc8aa92)

* Correct icon image height

Kept parent div class the same.

(cherry picked from commit 61fa0d3)

* fix lint errors

* Add as next version

Co-authored-by: Jamie Pezone <[email protected]>
* replace weather.home usage with demo sensors
* add support for CDE forwarding (local proxy) + forward 5000 & 8123
* add extra ui graph with color_threshholds edge-case
@akloeckner
Copy link
Collaborator

Thanks for picking this up! From first sight and without testing, I'd suggest a few changes:

  • PRs should target the dev branch as far as I understood. So, maybe rebase it to get it merged more easily...
  • I believe, all computations can be delegated to computeState, by passing entity's index and using defaults (||). See computeUom for example. Right now, I believe, extrema are even scaled twice, i.e., when they are updated in updateExtrema and when they are displayed using computeState.
  • Take care to use the right precedence: entity || card || default. Right now, it looks to me as if card settings would take precedence over entity settings in computeState.
  • There are some typos resulting in multipler instead of multiplier.

@X-Ryl669
Copy link
Author

X-Ryl669 commented Dec 7, 2022

  1. The dev branch is lagging behind the master branch. Is it expected ?
  2. That's what I did at first, but it doesn't work, since computeState is also called for non entity (like bounds). I could have broken this to 2 different functions but I think it's cleaner this way. Extrema aren't scaled twice, I'm just saving the value multiplier used when computing the extrema in the abs array, so I can compute their state with the appropriate multiplier when required.
  3. You're right, I'll fix this.
  4. I had a doubt about this from the initial PR which used multipler. TBH, I don't like the name "value multiplier" and I think "scale factor" would be better, what do you think ?

@X-Ryl669 X-Ryl669 changed the title feat: add value_multiplier and extend value_factor (next) feat: add scale_factor and extend value_factor (next) Dec 7, 2022
@X-Ryl669
Copy link
Author

X-Ryl669 commented Dec 7, 2022

Closed to target dev branch in #884. I've renamed the variable to scaleFactor and changed the config order as expected.

@X-Ryl669 X-Ryl669 closed this Dec 7, 2022
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

Successfully merging this pull request may close these issues.

None yet

7 participants