GCI "Filter layers that are exclusive to selected frameworks" #418

ZeroZeroJedenJeden · 2018-11-01T17:16:49Z

Added filters to show/hide layers in layers bar to selected framework.

thatbrguy · 2018-11-01T17:36:28Z

Hey @ZeroZeroJedenJeden few suggestions before I do a full review.

Can you update the instructions in this documentation to add their layer name to your variables?
Please share a gif or video demonstrating your feature in the Fabrik Gitter channel.

thatbrguy

@ZeroZeroJedenJeden Excellent work overall, couple of code-wise suggestions:

thatbrguy · 2018-11-02T14:19:13Z

ide/static/js/filterbar.js

+              <div className="form-group pull-right">
+                <div className="dropdown">
+                  <button id="topbar-icon" className="btn btn-default dropdown-toggle form-control" data-toggle="dropdown">
+                    <span className="glyphicon glyphicon-list-alt" aria-hidden="true"></span>


Suggested change

<span className="glyphicon glyphicon-list-alt" aria-hidden="true"></span>

<span className="glyphicon glyphicon-filter" aria-hidden="true"></span>

I think this should look better

thatbrguy · 2018-11-02T14:19:44Z

ide/static/js/pane.js

      );
  }
 }
+
+// class FilterBar extends React.Component {


Please remove comments if you're sure about your changes.

thatbrguy · 2018-11-02T14:21:15Z

ide/static/js/content.js

@@ -1310,6 +1311,7 @@ class Content extends React.Component {
             <Login setUserId={this.setUserId} setUserName={this.setUserName}></Login>
             <h5 className="sidebar-heading insert-layer-title">
              <input id="layer-search-input" placeholder="Search for layer"></input>
+			  <FilterBar />


Please resolve the below webpack error (line 1314):

content.js:1314:2 error Mixed spaces and tabs no-mixed-spaces-and-tabs

thatbrguy · 2018-11-02T14:22:38Z

ide/static/js/filterbar.js

+                  </ul>
+                </div>
+              </div>
+			  </div>


Please resolve the below webpack error (lines 96 and 97):

filterbar.js:96:2 error Mixed spaces and tabs no-mixed-spaces-and-tabs filterbar.js:97:2 error Mixed spaces and tabs no-mixed-spaces-and-tabs

thatbrguy · 2018-11-02T14:24:58Z

ide/static/js/pane.js

+                  // </ul>
+                // </div>
+              // </div>
+			  // </div>


Please resolve the below webpack error (lines 560 and 561). Just removing the comments would do as well, but mentioning it anyway:

pane.js:560:2 error Mixed spaces and tabs no-mixed-spaces-and-tabs pane.js:561:2 error Mixed spaces and tabs no-mixed-spaces-and-tabs

thatbrguy · 2018-11-02T14:33:14Z

@ZeroZeroJedenJeden That's some excellent work, well done! I have a few comments based on my observations:

While using the Keras filter, layers like "Input" and "Convolution" get filtered out. However, those layers are available in Keras. Please check your filter arrays if they are missing/having any irrelevant elements.
It would great if you could have the checkmark like: Keras [X] instead of [X] Keras.
Currently I have to click the checkmark area to toggle the filter. Can you change it such that clicking anywhere on the Keras [X] dropdown would toggle the filter (Instead of clicking on the checkmark [X])?
Your functionality works well in conjunction with the search bar. So that's great!

@Ram81 @yashdusing It'll be great if you guys can review this functionality as well.

ZeroZeroJedenJeden · 2018-11-02T15:51:52Z

"Input" and "Convolution" added to filter array.
[X] Keras changed to Keras [X]
checkmark work on all button area.
removed all comented code and removed all error Mixed spaces and tabs no-mixed-spaces-and-tabs errors

@thatbrguy If you can, check the work.

yashdusing · 2018-11-02T17:15:22Z

The functionality's great. But I think :

You should add some margin to filter layer's search bar in the bottom and the top. This looks a little clustered.
Try to align the button along with search bar. The search bar takes up both the Insert layer heading's area as well as search bar's. Try to bring it to search bar's level and keep it within that bounds.
Apart from this : LGTM

ZeroZeroJedenJeden · 2018-11-02T18:33:47Z

@yashdusing I see it, but to do it, I'll modify search bar placing system. Search bar is not my GCI task, so I wait what @thatbrguy will say.

thatbrguy · 2018-11-03T04:10:42Z

@ZeroZeroJedenJeden Hey, good progress! However there are still some layer discrepancies. For instance, Crop is Caffe only, but is available on Keras and TensorFlow as well. Similarly, LRN is available in Keras, but not displayed. Input is available on TensorFlow, but not displayed as well, and so on.

Please go through data.js and the files in tensorflow_app, keras_app and caffe_app and verify if everything has been accounted for. We'll focus on this for now. If time permits, we can work on @yashdusing's suggestion

ZeroZeroJedenJeden · 2018-11-03T10:52:13Z

My brain has stopped working. I got elements for filter from #409 PR without verification, where Input missing, LRN only for cafee and Crop for all frameworks. I going to rewrite all arrays with official domumentacions.

ZeroZeroJedenJeden · 2018-11-03T12:11:49Z

Updated filters from data.js

ZeroZeroJedenJeden · 2018-11-03T13:30:16Z

Modifed placing system.

thatbrguy · 2018-11-03T17:35:14Z

@ZeroZeroJedenJeden Glad that you're actively verifying the layers, good work! @yashdusing Could you verify if the filter functionality has all the proper layers for each framework? I may have missed something.

I got a few points as well:

Looks like content.js, pane.js and filterbar.js got added twice in your commits. Please remove the redundancies.
Placement looks weird all of a sudden, did you change anything? You can modify it to the older version if you cant fix it. I've attached an image showing the problem below:

yashdusing · 2018-11-03T17:43:38Z

I have the same display issue as @thatbrguy so please fix that bug.

ZeroZeroJedenJeden · 2018-11-03T18:06:04Z

I pushed content.js, pane.js and filterbar.js to master folder. This create this look bug and double files. It should be good now. @thatbrguy If you can, check this.

thatbrguy · 2018-11-03T18:37:36Z

@ZeroZeroJedenJeden Hello. Yes, placement looks perfect now.

About the layers, @yashdusing and I will verify if everything has been properly added and let you know if something's missing.

yashdusing · 2018-11-04T12:08:46Z

@ZeroZeroJedenJeden Please look into the comments I've made and fix those. Apart from those comments : LGTM

yashdusing · 2018-11-03T18:31:08Z

ide/static/js/filterbar.js

+      "Bidirectional_Button", "RepeatVector_Button", "Masking_Button", "Permute_Button", "InnerProduct_Button",
+      "Deconvolution_Button", "Regularization_Button", "Softsign_Button", "Upsample_Button", "Pooling_Button",
+      "LocallyConnected_Button", "Input_Button", "Convolution_Button", "LRN_Button", "DepthwiseConv_Button", "Flatten_Button",
+      "Reshape_Button", "Concat_Button", "Softplus_Button", "HardSigmoid_Button", "Dropout_Button"];


https://keras.io/layers/merge/
Also, these correspond to eltwise , I think

yashdusing · 2018-11-04T11:34:48Z

ide/static/js/filterbar.js

+      "InnerProduct_Button", "Deconvolution_Button", "ELU_Button", "Dropout_Button",
+      "Pooling_Button", "LocallyConnected_Button", "Sigmoid_Button",
+      "Convolution_Button", "TanH_Button",
+      "Input_Button", "Flatten_Button", "Reshape_Button", "Concat_Button"];


List of supported TF layers (as per dict) : Input, Convolution, Pooling, DepthwiseConv, InnerProduct, LRN, Concat, Flatten, BatchNorm, DeConv, Sigmoid, Softplus, Softsign, Elu, Relu, Softmax, TanH, Selu, Dropout, Eltwise

Remove the existing layers and put in these. Also, go through 'import_graphdef.py' once just in case I missed any layers .

yashdusing · 2018-11-04T12:01:41Z

ide/static/js/filterbar.js

+      "Reshape_Button", "BatchReindex_Button", "Split_Button", "Concat_Button", "Eltwise_Button", "Filter_Button",
+      "Reduction_Button", "Silence_Button", "ArgMax_Button", "Softmax_Button", "MultinomialLogisticLoss_Button",
+      "InfogainLoss_Button", "SoftmaxWithLoss_Button", "EuclideanLoss_Button", "HingeLoss_Button",
+      "SigmoidCrossEntropyLoss_Button", "Accuracy_Button", "ContrastiveLoss_Button", "Data_Button", "Crop_Button"];


You forgot Threshold Relu, Slice, Python layers. Apart from these, I didn't find any layers that were missed.

ZeroZeroJedenJeden · 2018-11-04T14:22:00Z

@yashdusing So I have problem. I can't find Slice button in pane.js. Does Slice have different name?

thatbrguy · 2018-11-04T15:52:09Z

@ZeroZeroJedenJeden Looks good to me, excellent work 👍

Ram81 · 2018-11-05T08:59:28Z

@ZeroZeroJedenJeden style gets messed up on chrome, can you fix this.

Ram81

@ZeroZeroJedenJeden Minor changes. Functionality wise LGTM

Ram81 · 2018-11-05T09:00:37Z

ide/static/js/content.js

@@ -1308,17 +1309,20 @@ class Content extends React.Component {
             />
             <h5 className="sidebar-heading">LOGIN</h5>
             <Login setUserId={this.setUserId} setUserName={this.setUserName}></Login>
+             <h5 className="sidebar-heading">
+             <div id="insert-layer-sign">INSERT LAYER</div>


Correct the Indentation

Ram81 · 2018-11-05T09:01:15Z

ide/static/js/content.js

             </h5>
             <Pane
             handleClick = {this.handleClick}
             setDraggingLayer = {this.setDraggingLayer}
             />
             <div className="text-center">
-              <Tabs selectedPhase={this.state.selectedPhase} changeNetPhase={this.changeNetPhase} />
+             <Tabs selectedPhase={this.state.selectedPhase} changeNetPhase={this.changeNetPhase} />


Fix the indentation

Ram81 · 2018-11-05T09:02:37Z

ide/static/js/filterbar.js

+      "Deconvolution_Button", "Regularization_Button", "Softsign_Button", "Upsample_Button", "Pooling_Button",
+      "LocallyConnected_Button", "Input_Button", "Convolution_Button", "LRN_Button", "DepthwiseConv_Button", "Flatten_Button",
+      "Reshape_Button", "Concat_Button", "Softplus_Button", "HardSigmoid_Button", "Dropout_Button", "Eltwise_Button"];
+      var TensorFlowLayers = ["Input_Button", "Convolution_Button", "Pooling_Button", "DepthwiseConv_Button", "InnerProduct_Button",


Add a new line above this

Ram81 · 2018-11-05T09:02:44Z

ide/static/js/filterbar.js

+      "LRN_Button", "Concat_Button", "Flatten_Button", "BatchNorm_Button", "Deconvolution_Button", "Sigmoid_Button", 
+      "Softplus_Button", "Softsign_Button", "ELU_Button", "ReLU_Button", "Softmax_Button", "TanH_Button", "SELU_Button",
+      "Dropout_Button", "Eltwise_Button"];
+      var CaffeLayers = ["ImageData_Button", "HDF5Data_Button", "HDF5Output_Button", "Input_Button", "WindowData_Button",


Add a newline above this

Ram81 · 2018-11-05T09:02:58Z

ide/static/js/filterbar.js

@@ -0,0 +1,103 @@
+import React from 'react';


Rename the file to filterBar.js

Ram81 · 2018-11-05T09:04:01Z

ide/static/js/filterbar.js

+      var CheckBoxC = document.getElementById("CheckBoxC");
+      var visible = [];
+
+    let CheckBox = document.getElementById(cbid);


Follow the camel case style for variable names

Ram81 · 2018-11-05T09:04:21Z

ide/static/js/filterbar.js

+         visible = visible.concat(CaffeLayers);
+      }
+
+            for (let elem of $('.drowpdown-button')) {


Uneven Indentation

Ram81 · 2018-11-05T09:04:54Z

ide/static/js/filterbar.js

+                  </ul>
+                </div>
+              </div>
+              </div>


Again Indentation fixes

Ram81 · 2018-11-05T09:06:05Z

ide/static/css/searchbar_style.css

@@ -16,8 +17,8 @@

 #layer-search-input {
 	position: absolute;
-	top: 36px;
-	left: 20px;
+	top: 2px;


Rather than hardcoding the values you can assign relative values.

@ZeroZeroJedenJeden try using relative values to see if that solves the layout bug

Ram81 · 2018-11-05T09:08:19Z

ide/static/js/filterbar.js

+                if(id == visible[j]){
+                    elem.classList.remove("hide");
+                    j = visible.length + 1;
+                }else{


Put else on newline and add space after else

Ram81 · 2018-11-05T09:09:02Z

ide/static/js/filterbar.js

+            for (let elem of $('.drowpdown-button')) {
+                for (let j = 0; j < visible.length; j++){
+                let id = elem.id;
+                if(id == visible[j]){


space after ) and before {

Ram81 · 2018-11-05T09:11:05Z

ide/static/js/filterbar.js

+                    <li>
+                        <a className="btn" onClick={this.changeEvent.bind(this, "CheckBoxA")}>
+                        <label className="filter">Keras</label>
+                        <input type="checkbox" id="CheckBoxA" value="A" onClick={this.changeEvent.bind(this, "CheckBoxA")} />


Rename checkbox ids to be more specific for functionality

ZeroZeroJedenJeden · 2018-11-05T16:06:55Z

@Ram81 About this "style bug" on chrome. On my chrome all looks good. Try clean chrome cache, like ctrl + R and ctrl + F5 or clean site cache in chrome settings.

Ram81 · 2018-11-06T06:54:29Z

@ZeroZeroJedenJeden the size of filter button still looks to big. Is it same for you?

ZeroZeroJedenJeden · 2018-11-06T15:53:43Z

@Ram81 The size of filter button is the same size as topBar buttons (import, export etc).

yashdusing · 2018-11-06T15:59:27Z

Hello @ZeroZeroJedenJeden ! It seems that this task is still not yet done and you have simultaneously taken up another task for fixing layer chopiness. We would like you to finish this task up first and have the PR approved before taking up the other one as it violates the rule of taking up more than one tasks at a time.

ZeroZeroJedenJeden · 2018-11-06T16:22:21Z

@yashdusing Right, but after @thatbrguy "LGTM"comment I could take next task on GCI site. I took next task and after this @Ram81 wrote comments to this task.

yashdusing · 2018-11-06T16:55:27Z

@ZeroZeroJedenJeden sorry for the confusion. I thought that the PR wasn't approved so the task might not have the approval. You can continue your work on the layer chopiness. Ask for help if you're stuck

ZeroZeroJedenJeden · 2018-11-06T17:49:18Z

To @Ram81 big button comment. Are this look better?

Ram81

@ZeroZeroJedenJeden LGTM 👍

ZeroZeroJedenJeden added 2 commits November 1, 2018 17:40

Add files via upload

03115b8

Add files via upload

807afb6

ZeroZeroJedenJeden added 7 commits November 1, 2018 19:01

Update adding_new_layers.md

1d4a783

Update adding_new_layers.md

db576e3

Add files via upload

7e197ff

Add files via upload

555e8f1

Add files via upload

071aebd

Add files via upload

4ad5a1f

Update adding_new_layers.md

3503fad

thatbrguy suggested changes Nov 2, 2018

View reviewed changes

ZeroZeroJedenJeden added 3 commits November 2, 2018 16:44

Add files via upload

5eff333

Add files via upload

94ffe32

Update filterbar.js

e13de94

Add files via upload

6223c70

ZeroZeroJedenJeden added 2 commits November 3, 2018 14:28

Add files via upload

603cef9

Add files via upload

ec4781c

ZeroZeroJedenJeden added 3 commits November 3, 2018 18:57

Delete filterbar.js

22c835d

Delete pane.js

24d347a

Delete content.js

244980b

ZeroZeroJedenJeden added 2 commits November 3, 2018 18:58

Add files via upload

6b48678

Update content.js

e053568

yashdusing reviewed Nov 4, 2018

View reviewed changes

Add files via upload

cd40ef0

thatbrguy mentioned this pull request Nov 4, 2018

Added Slice layer to pane.js #425

Merged

Ram81 requested changes Nov 5, 2018

View reviewed changes

Ram81 reviewed Nov 5, 2018

View reviewed changes

ZeroZeroJedenJeden added 3 commits November 5, 2018 17:54

Add files via upload

ad4dfa9

Add files via upload

18532a0

Delete filterbar.js

bc81f8d

ZeroZeroJedenJeden and others added 3 commits November 6, 2018 20:40

Add files via upload

702aca3

Add files via upload

9ba2d18

Merge branch 'master' into master

95d250c

Ram81 approved these changes Nov 7, 2018

View reviewed changes

Ram81 merged commit 4b54694 into Cloud-CV:master Nov 7, 2018

	<span className="glyphicon glyphicon-list-alt" aria-hidden="true"></span>
	<span className="glyphicon glyphicon-filter" aria-hidden="true"></span>

GCI "Filter layers that are exclusive to selected frameworks" #418

GCI "Filter layers that are exclusive to selected frameworks" #418

Conversation

ZeroZeroJedenJeden commented Nov 1, 2018

thatbrguy commented Nov 1, 2018

thatbrguy left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatbrguy commented Nov 2, 2018

ZeroZeroJedenJeden commented Nov 2, 2018

yashdusing commented Nov 2, 2018

ZeroZeroJedenJeden commented Nov 2, 2018

thatbrguy commented Nov 3, 2018 • edited Loading

ZeroZeroJedenJeden commented Nov 3, 2018

ZeroZeroJedenJeden commented Nov 3, 2018

ZeroZeroJedenJeden commented Nov 3, 2018

thatbrguy commented Nov 3, 2018

yashdusing commented Nov 3, 2018

ZeroZeroJedenJeden commented Nov 3, 2018 • edited Loading

thatbrguy commented Nov 3, 2018

yashdusing commented Nov 4, 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

ZeroZeroJedenJeden commented Nov 4, 2018

thatbrguy commented Nov 4, 2018

Ram81 commented Nov 5, 2018

Ram81 left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ram81 Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeroZeroJedenJeden commented Nov 5, 2018

Ram81 commented Nov 6, 2018

ZeroZeroJedenJeden commented Nov 6, 2018

yashdusing commented Nov 6, 2018

ZeroZeroJedenJeden commented Nov 6, 2018

yashdusing commented Nov 6, 2018

ZeroZeroJedenJeden commented Nov 6, 2018

Ram81 left a comment

Choose a reason for hiding this comment

thatbrguy commented Nov 3, 2018 •

edited

Loading

ZeroZeroJedenJeden commented Nov 3, 2018 •

edited

Loading

Ram81 Nov 5, 2018 •

edited

Loading