116 dependent #176

joshmutus · 2016-05-09T18:31:23Z

Adds the ability to select which plots will be shown

Click the button on the middle right, below the reset zoom (home button)

Select which traces you want to see:

Press OK:

Will implement ALL or NONE for asssiting in selecting on large datasets. For now buttons are disabled

@DanielSank @jwenner @maffoo

add dialog for selecting traces Have to wire them together

essentially works now

jwenner · 2016-05-09T19:44:05Z

While I'd like to be able to test this, all the computers I have access to are Windows computers. Could you please do a setup on a Linux computer? (Either that or I can't test this). Note I'd like to see how the selection menu handles long lists of dependent variables (see any of the DAC ScanPhases datasets for instance - example either at ['','Test','GHzFPGA','Ivan DAC 59','151030','0_0',3] or see Austin for a dataset at Google). I'd also like to see what you do in the case of a 2D dataset, where the menu icon is already used for "draw with vargrid"

But overall, I like the idea as seen in the photos you provided. Don't know enough to comment on the code, though.

joshmutus · 2016-05-09T20:52:15Z

Thanks for taking a look Jim. I don't know how to deploy this branch on Matrix-reloaded for testing so I'll wait until this gets through review. If you have any suggestions from what you can see of the GUI in the screenshots, let me know

jwenner · 2016-05-09T21:09:35Z

@joshmutus, could you please post screenshots of the following:

What happens to the menu when there's a lot of dependent variables. See the GHzDAC ScanPhases I mentioned above for examples.
What happens to the buttons in the case of a 2D dataset? Example: ['','Jim','AliceBobRes','Cooldown160406','160503','7-T1 Stability'].
What is the text which appears when you hover the mouse over the button?
What happens when the label of the traces does not match that of the first trace? For the example you showed, what happens to the y-axis label if you selected traces 1 and 3 (i.e., the two phases)?

joshmutus · 2016-05-09T21:19:10Z

What happens to the menu when there's a lot of dependent variables. See the GHzDAC ScanPhases >I mentioned above for examples.

I hadn't thought of this. I'll make a it a scrollable dialog so they'll all show up.

What happens to the buttons in the case of a 2D dataset? Example:
['','Jim','AliceBobRes','Cooldown160406','160503','7-T1 Stability'].

Nothing. Not implemented yet. What do we want the behavior to be? I forget can we have multiple deps for 2 indeps? It's been a LONG time since I've taken data :/

What is the text which appears when you hover the mouse over the button?

"select trace"

What happens when the label of the traces does not match that of the first trace? For the example you > showed, what happens to the y-axis label if you selected traces 1 and 3 (i.e., the two phases)?

Good catch. Not the correct thing. I'll fix that.

joshmutus · 2016-05-09T21:35:41Z

@jwenner So I've changed the behavior for the axis label for selecting traces.

If you have multiple traces with multiple labels and units, the axis label will take on the value of the lowest order of the selected trace.

For example if I have
1 - magnitude
2 - phase
3 - magnitude
4 - phase

and I select traces 2 and 4, the label will show phase (with the legend and units fields filled in)
If I select traces 1 and 3 it will label magnitude.
If I select traces 3 and 4 it will label magnitude
If I select traces 2 and 3 it will label phase

I worry displaying lots of axis labels will get cluttered, so I'm just displaying one right now. Is that behavior good for now?

Screenshot for posterity (Yes I know plot isn't re-scaling, I'm following the breadcrumbs on that now) :

jwenner · 2016-05-09T21:44:23Z

Seems reasonable to me. I think this is akin to what the old Delphi grapher does (but I don't know the particular ordering that it uses). But you should definitely leave a comment in the code to this effect.

Long term, I don't know what a good solution is. One conceivable idea is to only plot traces with the same label as the first selected trace? Or to show all labels but color-coded? Or something else? Maybe @DanielSank or @maffoo has an idea.

But as for now, I think that what you propose for the axis label should work.

joshmutus · 2016-05-09T21:45:51Z

Also, just fixed the dialog so it will be scrollable for lots of dependents. I don't have a dataset locally with very many dependents, so I can't post a nice screenshot, but it works if I scrunch the screen up to be super duper small :)

jwenner · 2016-05-09T21:51:02Z

Since at least Austin has done the DAC ScanPhases script at Google, you all probably have a dataset with lots of dependents there. Typical location is something like ['','Test','GHzDAC', boardName,...].

joshmutus · 2016-05-09T21:51:40Z

I actually have no idea how to deploy on that machine either :(

joshmutus · 2016-05-09T22:17:42Z

Plot limits now change with selected traces:

All Traces

Phase:

Mag:

joshmutus · 2016-05-09T22:23:15Z

Thanks for the UI feedback @jwenner ! Very handy peer review even without knowing the code.

jwenner · 2016-05-09T22:37:22Z

From what you've said and the screenshots you've posted, a provisional LGTM (provisional based on not having tried it myself and not knowing the code). Although, we should either not close #116 or raise a new issue until this is made available in the 2D scan case as well.

jwenner · 2016-05-09T22:41:32Z

Thinking about it, one additional idea. On the "Select Traces" menu, would it be good to show both the label and legend (so "Magnitude S11" instead of "Magnitude")? That would at least provide a way to distinguish between two magnitudes, probabilities, etc.

Also, do the traces show up in the same order between the "Select Traces" menu and the list of dependent variables on the page which shows datasets/dependents/parameters?

joshmutus · 2016-05-09T22:45:59Z

Sure that's easy to add

joshmutus · 2016-05-09T23:06:09Z

added the thing.

I don't really feel comfortable merging before someone takes a look at the code. Also, even if it gets merged I don't know how to deploy to matrix-reloaded.

@DanielSank @ejeffrey do you know how to build and deploy on matrix reloaded?

jwenner · 2016-05-09T23:08:50Z

client-js/app/elements/labrad-plot.html

@@ -70,8 +71,8 @@
      </div>
      <div id="selectButtons">
      <!-- TODO implement these  -->
-        <paper-button disabled>ALL</paper-button>
-        <paper-button disabled>NONE</paper-button>
+        <paper-button on-click="selectAll">ALL</paper-button>


What do these buttons do? Do they plot all traces or just select all traces in the menu? If the latter, then they should be "Select ALL" and "Select NONE".

Sure, I guess it's more clear that way.

joshmutus · 2016-05-16T17:15:11Z

@maffoo can you take a look when you get a chance. I just fixed the way the checkboxes are read. It should log what traces are selected and which are added to this.displayTraces let me know if it works now.

maffoo · 2016-05-16T18:24:35Z

@joshmutus, works, though I get a typescript compiler error on lines 611 and 612. I think you need to cast to the appropriate element type:

if ((<HTMLInputElement>checkbox).checked) {
  selected.push((<HTMLInputElement>checkbox).dataIndex);
}

maffoo · 2016-05-16T18:25:09Z

BTW, I wish there were a better way to do that...

joshmutus · 2016-05-16T20:06:15Z

Cool. I noticed the errors, just wanted to make sure it's functional.

On Mon, May 16, 2016, 11:25 AM Matthew Neeley notifications@github.com
wrote:

BTW, I wish there were a better way to do that...

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#176 (comment)

joshmutus · 2016-05-17T05:18:18Z

Pushed to the branch addressing most of the comments. I'm having trouble casting the radio button types:

app/elements/labrad-plot.ts(617,56): error TS2339: Property 'dataIndex' does not exist on type 'HTMLElement'.
app/elements/labrad-plot.ts(623,58): error TS2339: Property 'selected' does not exist on type 'HTMLElement'.

AFAICT there are no HTMLElements with the properties selected or dataIndex, those come from IronSelectableBehavior, which isn't included in the polymer-ts typings. Should I just make that typing manually? Where do I put it?

joshmutus · 2016-05-17T20:22:03Z

So I just renamed tab-index because it exists on <HTMLInputElement> and is of the correct type. Is that reasonable?

maffoo · 2016-05-17T20:36:22Z

I don't think you want to use tab-index to just store some data, since that is meant for controlling tabbing in the UI. It worked fine for me to use dataIndex and checked with a cast to <HTMLInputElement> as in the snippet I posted earlier.

joshmutus · 2016-05-17T21:24:34Z

@maffoo
Data Index doesn't work for me on ```

app/elements/labrad-plot.ts(617,56): error TS2339: Property 'dataIndex' does not exist on type 'HTMLInputElement'.

joshmutus · 2016-05-17T21:29:56Z

So, this quiets the compiler and allows me to create my own custom properties:

I made a new class:

class CustomSelector extends HTMLElement {
  //Is this a hack?
  selected: string;
  dataIndex: number;
}

And I call
(<CustomSelector>radio).selected and (<CustomSelector>checkbox).dataIndex

Is that a total hack?

maffoo · 2016-05-17T21:36:00Z

Technically, DOM attributes that start with data- are supposed to be accessed in javascript through the dataset field, so this should be

selected.push((<HTMLInputElement>checkbox).dataset.index);

Does that work for you? I tried that and while the typescript compiler accepted it, it didn't work at run time. I'm just curious if that works for you. I'm guessing polymer is doing something non-standard with data attributes. Also, it would appear that you and I have different typescript and/or other library versions and maybe that is the source of the different behavior we're seeing here.

Rather than create a class just to cast to it, I would suggest just casting to <any> and adding a comment with a TODO to come back and investigate this further.

joshmutus · 2016-05-17T21:45:31Z

That neither works for me at runtime or compiles cleanly:

app/elements/labrad-plot.ts(617,64): error TS2339: Property 'index' does not exist on type 'DOMStringMap'

maffoo · 2016-05-17T22:29:19Z

Ok, I figured out how to make this work with data- attributes.

First, you have to use "explicit attribute binding" in labrad-plot.html, as described here:

<paper-checkbox checked name="traces" data-index$="{{index}}">{{index}}</paper-checkbox>

Then, you need to use square-bracket indexing to get at items inside the dataset object in labrad-plot.ts (the type definiton for DOMStringMap is here):

selected.push(Number(checkbox.dataset['index']));

All that being said, I'm not sure using a data- attribute like this is really worth it given that polymer already has mechanisms to pass things through the DOM to javascript.

How about this: use a new attribute name, say, trace-index and then just cast to <any> to make typescript happy:

in labrad-plot.html:

<paper-checkbox checked name="traces" trace-index="{{index}}">{{index}}</paper-checkbox>

in labrad-plot.ts

selected.push((<any>checkbox).traceIndex);

Sorry for all the back and forth on this. I figure if we work out a reasonable pattern here we can just use it in the future. Does this latter proposal seem reasonable to you?

joshmutus · 2016-05-17T23:06:14Z

Thanks for figuring this out. Totally cool with the back and forth, I'm glad we can find a way to do this properly as I think we'll be having to expand on this feature in the near future for dataset specific plotting. I'll add all this as soon as I can.

One question, that DOMStringMap typing... should that already be in typescript? Do I have to upgrade it or do I have to add this typing manually?

maffoo · 2016-05-17T23:14:25Z

That DOMStringMap definition is in lib.d.ts which is bundled with typescript. Basically, it provides typescript definitions for all of the built-in browser APIs. You can find it inside node_modules if you're curious, but there's nothing you need to do to use these definitions.

joshmutus · 2016-05-17T23:38:55Z

Alright. I think I made all the changes you commented on. Please take a quick look when you get the chance.

Thanks for the input!

maffoo · 2016-05-18T00:26:03Z

Choosing a trace on a 2D plot does not work for me. If I choose trace 0, the console log shows [NaN] for selected traces and the plotting does not work. If I choose trace 1, the console log shows [1] and it does seem to plot the new trace, although the coloring is wrong; it seems the z data limits need to be recomputed in that case.

joshmutus · 2016-05-18T00:48:41Z

Hm.

You're on the latest commit of the branch? One of the earlier commits broke
2D trace selection.

At least the color scale should recompute each time...

On Tue, May 17, 2016, 5:26 PM Matthew Neeley notifications@github.com
wrote:

Choosing a trace on a 2D plot does not work for me. If I choose trace 0,
the console log shows [NaN] for selected traces and the plotting does not
work. If I choose trace 1, the console log shows [1] and it does seem to
plot the new trace, although the coloring is wrong; it seems the z data
limits need to be recomputed in that case.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#176 (comment)

maffoo · 2016-05-18T00:51:23Z

Yeah, I'm on the latest commit. The radio buttons display strange behavior as well; when the dialog opens neither is selected, and I can select both or neither options, which I wouldn't expect with a radio button.

joshmutus · 2016-05-18T01:08:14Z

I've seen all those problems in the past, but I'm not seeing them on my end. You sure you're on 68c0177??

Maybe polymer components are out of date? Do a bower install?

maffoo · 2016-05-18T03:47:11Z

After fiddling with bower packages, it works. LGTM

joshmutus · 2016-05-18T16:06:05Z

Cool, thanks for figuring that out. It's a bit troubling this is fragile in regards to the bower packages...

How do we feel about squashing and merging through the web interface on this repo?

maffoo · 2016-05-18T16:09:20Z

Merging through the web interface is fine by me. I just changed the repo settings to only allow squash merges.

joshmutus · 2016-05-18T16:29:25Z

Shoot, forgot to say this fixes 116

joshmutus added 5 commits May 8, 2016 21:38

Add array defining selected traces

07ca78e

add dialog for selecting traces Have to wire them together

Add ability to select traces

7306596

fix bug in selected array

ab801e0

essentially works now

Rough out all or none buttons

0169602

fix error in type definition

00a40e4

Fix formatting, remove superfluous debug logs

0ce99f8

Make yLabel change when new trace selected

ac774e2

Make dialog scrollable for large numbers of deps

1e5f230

fix indentation

c9e5883

Update plot limits every time plot is redrawn

b815e00

joshmutus closed this May 9, 2016

joshmutus reopened this May 9, 2016

Complete all/none checkbox selector. Add detail to trace labels

4c3a0f0

jwenner reviewed May 9, 2016
View reviewed changes

Fix checkbox selection according to comments

b6e2e63

change according to comments

84d8995

Fixed typings?

9255136

Add type coercions

68c0177

joshmutus merged commit 7b4f8a6 into master May 18, 2016

joshmutus mentioned this pull request May 18, 2016

Grapher: Add ability to plot one or some of dependent variables #116

Closed

adamcw deleted the 116-dependent branch August 2, 2016 23:11

116 dependent #176

116 dependent #176

Conversation

joshmutus commented May 9, 2016

jwenner commented May 9, 2016

joshmutus commented May 9, 2016

jwenner commented May 9, 2016

joshmutus commented May 9, 2016 • edited Loading

joshmutus commented May 9, 2016 • edited Loading

jwenner commented May 9, 2016

joshmutus commented May 9, 2016

jwenner commented May 9, 2016

joshmutus commented May 9, 2016

joshmutus commented May 9, 2016

joshmutus commented May 9, 2016

jwenner commented May 9, 2016

jwenner commented May 9, 2016 • edited Loading

joshmutus commented May 9, 2016

joshmutus commented May 9, 2016

jwenner May 9, 2016

Choose a reason for hiding this comment

joshmutus May 9, 2016

Choose a reason for hiding this comment

joshmutus commented May 16, 2016

maffoo commented May 16, 2016

maffoo commented May 16, 2016

joshmutus commented May 16, 2016

joshmutus commented May 17, 2016

joshmutus commented May 17, 2016

maffoo commented May 17, 2016

joshmutus commented May 17, 2016

joshmutus commented May 17, 2016

maffoo commented May 17, 2016

joshmutus commented May 17, 2016

maffoo commented May 17, 2016

joshmutus commented May 17, 2016

maffoo commented May 17, 2016

joshmutus commented May 17, 2016

maffoo commented May 18, 2016

joshmutus commented May 18, 2016

maffoo commented May 18, 2016

joshmutus commented May 18, 2016

maffoo commented May 18, 2016

joshmutus commented May 18, 2016

maffoo commented May 18, 2016

joshmutus commented May 18, 2016

joshmutus commented May 9, 2016 •

edited

Loading

joshmutus commented May 9, 2016 •

edited

Loading

jwenner commented May 9, 2016 •

edited

Loading