[WIP] add min/max limits to temp plot (closes #240) #289

mlp6 · 2018-05-08T01:56:17Z

@suyashkumar completely unsure if this is the correct way to this... almost definitely not the most elegant, or even remotely so, but hey... its JavaScript, what do you expect. ;) Please correct.

suyashkumar

Overall LGTM, but had some comments about using map and moving over these changes to the new more generalized Plot component! I should have removed the temp-plot component earlier once it was successfully switched out with the more general Plot component, my apologies for the delay there.

suyashkumar · 2018-05-09T03:18:50Z

software/cloud-dashboard/react-client/src/components/temp-view/temp-plot.js

@@ -38,6 +40,22 @@ class TempPlot extends Component {
 		return array;
 	};

+  filterDataMinMax = (array) => {


This seems reasonable, but for the implementation of this function, consider using the map function as described here which will allow you to apply a function f to each element e in an array with the output f(e) populating the corresponding index in an output array.

Yeah, I wanted to use map... But I wasn't in Python. Syntax. Bah.

suyashkumar · 2018-05-09T03:23:53Z

software/cloud-dashboard/react-client/src/components/temp-view/temp-plot.js

@@ -9,6 +9,8 @@ import Input from 'react-toolbox/lib/input';
 const LINE_COLORS = ['#e41a1c', '#377eb8', '#4daf4a', '#984ea3', '#ff7f00', '#ffff33']


also, as seen here the TempPlot component has actually been replaced by a more generalized Plot component which is used to render both the temperature plot and the bucket view plot so these changes should likely go into the plot component! My apologies for not promptly removing this legacy component right away. Since it seems like the Plot component is working fine I can go ahead and remove this component once I ensure there are no lingering dependencies.

suyashkumar · 2018-05-09T03:26:14Z

software/cloud-dashboard/react-client/src/components/temp-view/temp-plot.js

@@ -9,6 +9,8 @@ import Input from 'react-toolbox/lib/input';
 const LINE_COLORS = ['#e41a1c', '#377eb8', '#4daf4a', '#984ea3', '#ff7f00', '#ffff33']

 const defaultNumberOfPoints = 300;
+const tempMin = 0;


since these changes will be in the generalized plot component as described above, these should likely be input props to the component (perhaps minLimit and maxLimit). These values can be passed into the Plot component by the parent, as is seen here. Inside the Plot component, you can of course access the current prop value using syntax similar to the following this.props.minLimit

Would it be worth putting these in constants.js? I'll take a stab at using the correct class for this now...

oh wait, this plot component is also used for the bucket tips... that wouldn't make sense as a default for that.

* Aaron requested limits on the temperature y-axis plots ranging from [0:80] degrees. Implemented this through a YAxis domain option that will impose these hard limits if the data exceed these; otherwise the y-axis will be dynamically set based on the data. * Chose this as opposed to re-mapping the data points that fall outside of this range so that the temperature data array integrity is maintained.

mlp6 · 2018-05-09T19:52:20Z

Making some attempted changes just to get more familiar with the code base. Latest commit imposes YAxis limits, but I haven't captured setting this for the bucket tips yet (still trying to find where those data are processed right now). Obviously still a WIP...

mlp6 · 2018-05-21T17:15:40Z

This is broken right now... so definitely do not merge this in!

mlp6 · 2018-05-21T17:48:20Z

software/cloud-dashboard/react-client/src/components/temp-view.js

 		var tempData = [];
 		if (this.props.temps.data) {
            tempData = this.props.temps.data.map(currentItem => {
                return {...currentItem.temps, time: currentItem.time};
            });
            tempData = tempData.reverse()
        }
+
+    this.props.dataMin = (tempData.min() < tempDataMin ? tempDataMin : tempData.min());


@suyashkumar trying to set these here seems to break the plots... insights?

by breaking the plots, they just don't render

nvm - I realize this is broken b/c of trying to assign something to props incorrectly (I think)

ah yes, you should not be setting this components own props from within it -- the parent to this component passes in props to this component somewhat like function inputs.

Likely what you want to be doing is passing in dataMin and dataMax as input props to the plot itself right? In which case you would be passing them into PlotCard like data is being passed in on L56.

yep... I realized this was a broken approach. not surprised by that.

mlp6 · 2018-05-21T18:59:09Z

@suyashkumar help... in general things were working with this approach to both have the temp and bucket plots have appropriate y-axis scaling, but in refactoring the code, something happened where now I get plot timeouts. before I spend more time figuring this out, can you vet the approach to setting the y-axis limits this way?

mlp6 · 2018-05-21T18:59:32Z

also, the unit test is broken, but I think that it was calling things that the new plot component doesn't use anymore

suyashkumar · 2018-05-21T19:03:09Z

software/cloud-dashboard/react-client/src/components/plot.js

@@ -92,7 +92,7 @@ class Plot extends Component {
            <ResponsiveContainer width="94%" height={300}>
                <LineChart data={dataToShow}>
                    <XAxis dataKey="time" label="Date" interval={tickInterval} tickFormatter={this.formatDate} />
-                    <YAxis />
+                    <YAxis type="number" domain={[this.props.yAxisMinMax[0], this.props.yAxisMinMax[1]]}/>


props.yAxisMinMax is not available here yet, it needs to be passed into this Plot component from the PlotCard component and you should be good to use it!

mlp6 · 2018-05-21T19:06:30Z

Plots are now rendering, though the plots don't seem to be respecting the actual min/max limits correctly. That sometimes seems to change as a function of when the plot is getting reloaded (?). Not sure...

mlp6 · 2018-05-21T19:08:59Z

Two suggestions:

We might want to have the plot min/max have the ability to be updated on the web page and default to values instead of just being hard-coded.
The averages that are displayed still use the raw "bad" data points, which probably needs to also be fixed with some data scrubbers.

Thoughts @aforbis-stokes @Graham-H-Miller

suyashkumar · 2018-05-21T19:13:17Z

hmm may have to look deeper into that later, console.loging the min/max values may help?

mlp6 · 2018-05-21T19:29:33Z

figured it out, but will need some time to fix...

mlp6 · 2018-05-21T20:04:42Z

@suyashkumar I fixed the caluclation of the min/max y-axis values... they are properly console logged by from the plot component, but the actual rendered plot does not appear to be respecting the <Yaxis ... attribute that I am setting:

ADPL/software/cloud-dashboard/react-client/src/components/plot.js

Line 95 in 6f59239

    
           <YAxis type="number" domain={[this.props.yAxisMinMax[0], this.props.yAxisMinMax[1]]}/>

This could use your eyes when you have a chance...

mlp6 · 2018-05-21T20:05:34Z

software/cloud-dashboard/react-client/src/components/temp-view.js

+    var tempDataMax = 80;
+
+    var allTempData = [];
+    for (var i in tempData) {


@suyashkumar Better way to do this in Javascript?

mlp6 · 2018-05-21T20:05:59Z

software/cloud-dashboard/react-client/src/containers/bucket-view.js

@@ -72,6 +72,12 @@ class BucketView extends Component {
            }
        }

+    var flowDataValues = [];
+    for (var i in flowData) {


@suyashkumar better way to do this in Javascript?

mlp6 · 2018-05-21T20:06:28Z

software/cloud-dashboard/react-client/src/containers/bucket-view.js

+    for (var i in flowData) {
+      flowDataValues[i] = flowData[i].flow;
+    }
+    var yAxisMinMax = [Math.min.apply(Math, flowDataValues), Math.max.apply(Math, flowDataValues)];


@suyashkumar do you really have to do Math.min.apply(Math... to work with an array?!

It appears the recharts will override your YAxis domain unless you explicitly set `allowDataOverflow=True`.

aforbis-stokes · 2018-05-21T20:52:07Z

In regards to the hard limit/data scrubbing, voiding the faulty data would be better in my opinion for the reason you suggested for skewing averages.
That said, a quick-fix of hard limits is ok with me for the moment if data scrubbing is turning out a challenge. The plot shared on Slack does look not-ideal, but we recently came across a rare connector problem which has now been fixed. Most data that would be discarded is only a few points across a 3-day span and would not cause so many lines entering/leaving the limits.

mlp6 · 2018-05-21T20:54:39Z

@aforbis-stokes what would you like bad data points set to, just NaN? I'm hesitating to set them to 0 or 80 since if they are bad, there is no reason to give them actual value. I need to see what the recharts package that we're using for plotting will do with NaN values.

aforbis-stokes · 2018-05-21T20:57:15Z

I do think NaN is better than setting to 0 or 80. There is some value in knowing that data is funky, and the extreme numbers we see are good indicators of hardware issues. Is it possible to get something like "-NaN" for <0 and "Nan" or "+NaN" for >80? Typically a negative value means a probe is disconnected, while a very large number means things are connected, but something is damaged.

mlp6 · 2018-05-21T21:01:58Z

ok - I don't think that we can assign a sign to the NaN, but what we can do is when we scrub the data we can look for those trends and determine set a status for that thermistor based on the sign of the bad values and record those in the logs.

aforbis-stokes · 2018-05-21T21:02:52Z

That sounds good to me.

mlp6 · 2018-05-22T15:14:53Z

@suyashkumar will also need help updating the unit tests that you have for temp-view. Getting errors associated with:

ADPL/software/cloud-dashboard/react-client/src/test/components/temp-view/temp-view.test.js

Line 16 in 82dc672

ReactDOM.render(React.createElement(TempPlot, props), div);
ADPL/software/cloud-dashboard/react-client/src/test/components/temp-view/temp-view.test.js

Line 20 in 82dc672

const component = ReactTestUtils.renderIntoDocument(React.createElement(TempPlot, props));

I'm flying completely blind on what you are actually testing with those, but there appears to be something that has internally changed when you went from a temp-plot component to the more generalized plot component.

add min/max limits to temp plot (closes #240)

7c8a407

mlp6 added the enhancement label May 8, 2018

mlp6 requested a review from suyashkumar May 8, 2018 01:56

mlp6 added 2 commits May 7, 2018 21:57

fix js syntax

c695716

use tempMin/Max vars

5cd85f7

suyashkumar reviewed May 9, 2018

View reviewed changes

suyashkumar self-assigned this May 9, 2018

mlp6 added 2 commits May 9, 2018 15:30

rm legacy temp-plot component (replaced by more generic plot component)

d54b330

mlp6 added 2 commits May 9, 2018 16:31

update test to use Plot component

e0f6153

change TempPlot -> Plot component

4848a70

mlp6 added 2 commits May 21, 2018 13:31

fix random things that probably do not matter

f31cbf1

fix ; syntax

6084edf

mlp6 commented May 21, 2018

View reviewed changes

mlp6 added 4 commits May 21, 2018 14:27

rm legacy temp-plot component (replaced by more general plot)

6468e8e

add yAxisMinMax array for plots

c78086e

merge in temp plot test branch with this one

202aadf

use Math.min/max() since my array methods do not actually seem to exist

0b649eb

suyashkumar reviewed May 21, 2018

View reviewed changes

add yAxisMinMax to plot-card component

4a6fdac

fix calculation of yAxisMinMax values

6f59239

mlp6 commented May 21, 2018

View reviewed changes

fix y-axis plotting issue

92db7d2

It appears the recharts will override your YAxis domain unless you explicitly set `allowDataOverflow=True`.

mlp6 added 2 commits May 21, 2018 22:20

clean up ESLint warnings

5878f07

rm what appears to be an unused import

bb35826

mlp6 added 2 commits May 22, 2018 15:29

replace clunky loops with array reducers

0e2d9b0

add console logs to help debug tricky arrays

43f0969

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] add min/max limits to temp plot (closes #240) #289

[WIP] add min/max limits to temp plot (closes #240) #289

mlp6 commented May 8, 2018

suyashkumar left a comment

suyashkumar May 9, 2018

mlp6 May 9, 2018

suyashkumar May 9, 2018 •

edited

Loading

suyashkumar May 9, 2018

mlp6 May 9, 2018

mlp6 May 9, 2018

mlp6 commented May 9, 2018

mlp6 commented May 21, 2018

mlp6 May 21, 2018

mlp6 May 21, 2018

mlp6 May 21, 2018

suyashkumar May 21, 2018

mlp6 May 21, 2018

mlp6 commented May 21, 2018

mlp6 commented May 21, 2018

suyashkumar May 21, 2018

mlp6 May 21, 2018

mlp6 commented May 21, 2018

mlp6 commented May 21, 2018

suyashkumar commented May 21, 2018

mlp6 commented May 21, 2018

mlp6 commented May 21, 2018

mlp6 May 21, 2018 •

edited

Loading

mlp6 May 21, 2018

mlp6 May 21, 2018

aforbis-stokes commented May 21, 2018

mlp6 commented May 21, 2018

aforbis-stokes commented May 21, 2018

mlp6 commented May 21, 2018

aforbis-stokes commented May 21, 2018

mlp6 commented May 22, 2018

		@@ -9,6 +9,8 @@ import Input from 'react-toolbox/lib/input';
		const LINE_COLORS = ['#e41a1c', '#377eb8', '#4daf4a', '#984ea3', '#ff7f00', '#ffff33']

[WIP] add min/max limits to temp plot (closes #240) #289

Are you sure you want to change the base?

[WIP] add min/max limits to temp plot (closes #240) #289

Conversation

mlp6 commented May 8, 2018

suyashkumar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suyashkumar May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlp6 commented May 9, 2018

mlp6 commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlp6 commented May 21, 2018

mlp6 commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlp6 commented May 21, 2018

mlp6 commented May 21, 2018

suyashkumar commented May 21, 2018

mlp6 commented May 21, 2018

mlp6 commented May 21, 2018

mlp6 May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aforbis-stokes commented May 21, 2018

mlp6 commented May 21, 2018

aforbis-stokes commented May 21, 2018

mlp6 commented May 21, 2018

aforbis-stokes commented May 21, 2018

mlp6 commented May 22, 2018

suyashkumar May 9, 2018 •

edited

Loading

mlp6 May 21, 2018 •

edited

Loading