-
Notifications
You must be signed in to change notification settings - Fork 9
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
326 change scaling for the power import export graph #384
326 change scaling for the power import export graph #384
Conversation
5b3fd24
to
0452942
Compare
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.
It does seems odd that the scaling can suddenly jump, but if there's no way to determine the value range beforehand, I think this works well enough.
pages/BriefMonitorPanel.qml
Outdated
|
||
// If we show export power, the minimum scale of the y axis goes from -1000W to +1000W | ||
// If we don't show export power, the minimum scale of the y axis goes from 0W to +1000W | ||
readonly property int minimumRangeWatts: graphShowsExportPower |
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.
The naming improvements from the last PR help a lot, but to clarify, they don't all need to be separate properties, unless you think it helps for some structure/readability. E.g. maybe this could be folded into the initialization for graphPowerRange
as a named JS variable:
readonly property real graphPowerRange: {
const minimumRangeWatts = ...
return graphShowsExportPower ? Math.max(2 * peakPowerImportedOrExported, minimumRangeWatts) : Math.max(gridPower.maximum, minimumRangeWatts)
}
Also, given there's an assumption throughout this change that the importsAndExports range is twice that of importsOnly, maybe you don't need both Theme.animation.loadGraph.minimumScale.importsAndExports
and Theme.animation.loadGraph.minimumScale.importsOnly
to be in the Theme, could just put one of them in the theme and calculate the other one.
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.
That is much nicer, I think it's much more readable the way you suggest. Done wherever appropriate.
I found a bug, and have fixed it with this latest commit.
If our historical power data was like this: [-1000, 1000, -1000, 1000, ...],
and inputsPower.minimumSeen === -1000, and inputsPower.maximumSeen === 1000,
our graph 'y' values would be like this:
[-1, 0, -1, 1, ...]
If we then got a new power import reading of 5000W, we need to scale down all of the historical data by a factor of 5, eg.
[-0.2, 0.2, -0.2, 0.2, ...]
0452942
to
e9ca0ac
Compare
pages/BriefMonitorPanel.qml
Outdated
|
||
// If we have ever seen power exported to the grid, the graph shows imported and exported power, as in Graph B. | ||
// Otherwise, we only show imported power, as in Graph A. | ||
readonly property bool graphShowsExportPower: inputsPower.minimumSeen && inputsPower.minimumSeen < 0 |
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.
Is this supposed to be !isNaN(inputsPower.minimumeSeen) && inputsPower.minimumSeen < 0
? Otherwise with the bool coercion it effectively becomes inputsPower.minimumSeen != 0 && inputsPower.minimumSeen < 0
, which seems odd.
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.
Yes it is. I just realised that I can get rid of the 'isNan(...)' check as well, as comparing NaN with an integer always returns false anyway.
pages/BriefMonitorPanel.qml
Outdated
|
||
property real _oldGraphPowerRange: NaN | ||
|
||
function addNewValue() { |
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.
Looking at this again, I'm unsure about this mixed usage of imperative values and dynamic binding values. This function is sampling the power at this point in time, so perhaps minimumRangeWatts
, graphPowerRange
, normalizedZeroPowerPoint
and normalizedPower
should be calculated as variables within this function, instead of having them as dynamic properties that could change value at any time (outside of this sampling point)?
It might not make much difference in practice, as the binding values are not used until this function is called, but it would improve code readability to know that the values are not used outside of the sampling point.
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.
I have updated this as suggested. There are a few drawbacks to doing it this way, those properties can't be 'readonly' any more if I update them imperatively. Also, we have to be careful to make sure they are updated in the correct order, as the dependencies between the properties is complex. To my eyes, it seems less readable like this, what do you think?
I've committed this separately for now, in case you decide against it. If you decide you want the changes, I'll squash the commit.
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.
I'd just remove as many properties as possible and use const variables in the function instead. Most of the properties aren't used outside of this component anyway, and that way the significance of the order of updates only matters within the function.
The only property accessed from outside this component is normalizedPower
, for binding the slider value, and that value can just be set imperatively from this function instead.
Currently graphShowsExportPower
needs to be a property to trigger onGraphShowsExportPowerChanged, but I think that can be removed and the onGraphShowsExportPowerChanged code can be called conditionally, similarly to how scaleHistoricalData() is only called if the graph power range has changed.
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.
Updated as suggested, with the exception of graphShowsExportPower. If I changed this to be a const in the function, I would need another property 'oldGraphShowsExportPower' anyway in order to detect when it changed, and it just wouldn't buy us anything.
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.
Okay, that sounds good.
d2ea304
to
6a634f3
Compare
property real maximumValue: NaN // if NaN, the max is dynamically adjusted to the maximum encountered value | ||
property real maximumValue: NaN // if NaN, _max is dynamically adjusted to the maximum encountered value | ||
readonly property real maximumSeen: _max | ||
readonly property real minimumSeen: _min |
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.
I just realized, minimumSeen/maximumSeen is incorrect for these, as the _max can also be just the maximumValue, rather than the maximum value seen. So how about actualMinimum
and actualMaximum
instead? What do you think?
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.
Updated to handle the case where maximumValue !== NaN
6a634f3
to
571a77d
Compare
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.
Looks great 👍
|
||
if (_oldGraphPowerRange != graphPowerRange) { | ||
if (!isNaN(_oldGraphPowerRange)) { | ||
const scalingFactor = graphPowerRange / _oldGraphPowerRange |
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.
I assume _oldGraphPowerRange can't be zero here?
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.
that's right, it starts at 1000 and can only increase in value
// If we then got a new power import reading of 5000W, we need to scale down all of the historical data by | ||
// a factor of 5, eg. [-0.2, 0.2, -0.2, 0.2, ...] | ||
for (let i = 0; i < model.length; ++i) { | ||
model[i] = normalizedZeroPowerPoint + (model[i] - normalizedZeroPowerPoint) / scalingFactor |
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.
is model here a javascript property (an array), or a C++ thing? If it's a C++ thing, each element set here will cause whole-property writeback, as well as whole-property signal emission. Might not be a problem, but if it is, you might want to do something like:
var tempModel = model;
for (let i = 0; i < tempModel.length; ++i) tempModel[i] = normalizedZeroPowerPoint+(tempModel[i]-normalizedZeroPowerPoint)/scalingFactor;
model = tempModel;
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.
Ooooh, nice catch. It's a javascript array, and it's used as the model for 12 PathCubic objects in a ShapePath. I ran the debugger over it, and it seems to update the entire model, then it goes and updates each of the 12 PathCubic objects once. So, I think it's accidentally ok (I hadn't considered this). Would you prefer it updated anyway?
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.
no, it's fine as-is. I was just lazy and didn't check whether it was a javascript array.
// as the range has doubled, and the zero-point has changed from 0 to 0.5. | ||
for (let i = 0; i < model.length; ++i) { | ||
if (model[i] < 0.5) { | ||
model[i] = model[i]/2 + 0.5 |
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.
again just flagging the model element set causing whole-property-writeback possibility
571a77d
to
32ec76a
Compare
Prior to this change, the 'power' graph assumed that the system could export power, and reserved half of the vertical space for displaying exported power. This wasted half of the vertical space for systems that never export power. Now, if we haven't seen any power exported, the graph uses the entire vertical space to display imported power. If the system ever exports power, the graph changes so that it displays both imported and exported power. Prior to this change, the range of the graph automatically changed based on the maximum absolute power ever seen. This meant that if the most power ever imported was 20W, and was currently importing 20W, the graph would be showing a power import of 100%. Now, there is a minimum scale of 1000W, so the system described above would show a power import of 20/1000 = 2%.
32ec76a
to
c93b149
Compare
To test this change, run in mock mode, and press 't' once started to disable the timers that generate mock data. Then, open the brief page side panel to see the power graph. Press 'shift + P' to import more power, press 'p' to export more power.
There is a specific use case I'd like you to look at. Most of the time, you can disable the timers soon enough that the system hasn't exported any power yet. This is the one I want you to look at. Tap 'shift + P' to increase the power imported to ~1000W. Note that the graph is in 'export only' mode, i.e. it starts at the bottom. Tap 'p' to reduce power exported to 0, and keep tapping to export power until you are exporting ~1000W. Note that when you export power for the first time, the graph changes to 'import/export' mode, i.e. it starts in the middle, and goes up or down depending on whether you are importing or exporting. When it changes to 'import/export', it offsets and scales the history. There is a bit of lag, do you think it's good enough?