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

Updated UIdocumentation.md #497

Merged
merged 6 commits into from
Dec 11, 2018
Merged

Updated UIdocumentation.md #497

merged 6 commits into from
Dec 11, 2018

Conversation

khknopp
Copy link
Contributor

@khknopp khknopp commented Dec 9, 2018

I updated the UIdocumentation.md to include new features and reviewed the previous version.

* ```allowDrop```
* event
* ```clickLayerEvent``` - invoked on a click of a layer or on drag of that layer
* event
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kayo11 can you remove the list of parameters from all description

```checkIfCuttingLine``` is passed in a positional block that includes x and y coordinates (it assumes a px is at the end of each x and y) for each endpoint of the line it is checking.
Specifically, it is checking if the line formed with the coordinates in the positional block will cut into any other nodes between them.

```checkIfCuttingLine``` creates then creates an equation from the x and y points by calculating the slope and using point slope form.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the grammar

@khknopp
Copy link
Contributor Author

khknopp commented Dec 10, 2018

@Ram81 right on it.

@khknopp
Copy link
Contributor Author

khknopp commented Dec 10, 2018

@Ram81 Done, sorry for the grammar mistakes. I hope it's alright now.

(The bullets contain methods specific to the file along with their explanations.)

```addCommentModal.js``` is responsible for adding comments into the interface. It has three methods:
* ```handleClick```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No explanation for this method? Having no explanation looks a bit weird while all other methods have them. Either move this method to the end of the list, or provide a brief explanation.

* ```dismissError``` - dissmisses the error by ```errorIndex```
* ```addError``` - adds error using ```errorText```
* ```dismissAllErrors``` - dismisses all errors
* ```copyTrain``` - copies the nets train option for the test option
* ```trainOnly```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No explanation for this method? Having no explanation looks a bit weird while all other methods have them. Either move this method to the end of the list, or provide a brief explanation.

***
#### ```error.js```
```error.js``` contains Error React Component with two methods:
* ```dismissError```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No explanation for this method? Having no explanation looks a bit weird while all other methods have them. Either move this method to the end of the list, or provide a brief explanation.

@thatbrguy
Copy link
Contributor

@Kayo11 Hello. Neat work! I noticed that some methods do not have explanations. Either move them to the end of the list, or provide a one liner explanation of what they do (recommended). I have marked some in my review. I have may have missed some methods so please check the entire file.

@khknopp
Copy link
Contributor Author

khknopp commented Dec 10, 2018

@thatbrguy Yes, you're absolutely right. Thank you for the feedback. I'm on it

@khknopp
Copy link
Contributor Author

khknopp commented Dec 10, 2018

@thatbrguy I think I got all of them. Should I change anything else?


```canvas.js``` also contains the code that decides whether a node's line needs to be rerouted if it is cutting through another node.

#### ```canvas.js```'s placement algorithm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to visualisation algorithm instead of placement algorithm

* ```loadLayerShapes``` - AJAXs to backend to model parameters
* ```exportNet``` - AJAXs to backend and then passes back error/success
* framework
* ```exportPrep``` - invoked by ```exportNet```; prepares for model export
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use preprocessed model object for export

The method it uses is the following:
```checkIfCuttingLine``` is passed in a positional block that includes x and y coordinates (it assumes a px is at the end of each x and y) for each endpoint of the line it is checking.
Specifically, it is checking if the line formed with the coordinates in the positional block will cut into any other nodes between them.
* ```changeNetName``` - invoked on the event of net name changed; changes the name of the net
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not clear, can you rephrase this

```checkIfCuttingLine``` is passed in a positional block that includes x and y coordinates (it assumes a px is at the end of each x and y) for each endpoint of the line it is checking.
Specifically, it is checking if the line formed with the coordinates in the positional block will cut into any other nodes between them.
* ```changeNetName``` - invoked on the event of net name changed; changes the name of the net
* ```changeNetStatus``` - changes boolean for rebuilding.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rephrase this

Specifically, it is checking if the line formed with the coordinates in the positional block will cut into any other nodes between them.
* ```changeNetName``` - invoked on the event of net name changed; changes the name of the net
* ```changeNetStatus``` - changes boolean for rebuilding.
* ```changeNetPhase``` - changes the phase of the net
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

be more specific to what phase

* ```changeNetPhase``` - changes the phase of the net
* ```dismissError``` - dissmisses the error by ```errorIndex```
* ```addError``` - adds error using ```errorText```
* ```dismissAllErrors``` - dismisses all errors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can leave some of the obvious methods


The ```getBetween``` also serves the purpose of returning which direction in which the majority of the blocks between are. This is purely for performance, otherwise it would be seperated into a seperate function.
#### ```field.js```
```field.js``` contains the various different fields used by the layer editor.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the grammar

The ```jsplumb.js``` file contains code that handles the arrangement and the dragging/connecting of the layers. A new custom connector is created. There is an if function to check whether the node it is connecting is needing to be routed through an extension. A global variable stores this information, and the actual calculation is handled in ```checkIfCuttingNet``` and ```checkIfCuttingLine```. The global variable contains the amount of pixels it needs to move over, and it will contain direction it needs to go in (based on whether it is positive or
negative.)
#### ```importTextbox.js```
Renders the textbox for model import from text, using the three frameworks: Caffe, Keras and Tensorflow.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrase this

***

#### ```jsplumb.js```
The ```jsplumb.js``` file contains code that handles the arrangement and the dragging/connecting of layers. A new custom connector is created. There is an ```if``` function to check whether the node it is connecting to is needs to be routed through an extension. A global variable stores this information, and the actual calculation is handled in ```checkIfCuttingNet``` and ```checkIfCuttingLine```. The global variable contains the amount of pixels it needs to move over, and it will contain direction it needs to go in (based on whether it is positive or negative.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rephrase this too, some of the lines are not clear

### ```tooltip.js``` and ```tooltipData.js```

#### ```netLayout_vertical.js```
Defines the vertical align of network elements. ```netLayout_vertical.js``` uses a function, called ```allocatePosition``` to give the layers their position on the canvas. It finds the closest position available to the preferred position, and checks if a node is already on that position on the X axis (if one is, a position left or right is assigned). The file also uses a custom Topolical Sort to give the nodes there position (it is based on the inputs and outputs of nodes). Later it is checked if any nodes averlap and in case they do, they are moved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace align with alignment

@khknopp
Copy link
Contributor Author

khknopp commented Dec 10, 2018

Now, I changed the code, according to your suggestions, and fixed some other issues. I hope everything is fine after my changes.

@khknopp
Copy link
Contributor Author

khknopp commented Dec 11, 2018

@Ram81 @thatbrguy Is there anything else that needs a change?

#### ```topbar.js```
```topbar.js``` shows the top section of the sidebar. It has two methods:
* ```checkURL``` - checks model import URL
* ```render``` - renders the top section of the sidebar
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kayo11 as I said before no need to mention methods for which name of function clearly specifies the function

* ```handleKeyPress``` - handles layer delete after the 'delete' key is pressed
* ```componentDidMount``` - invokes ```handleKeyPress```
* ```componentWillUnmount``` - invokes ```handleKeyPress```
* ```render``` - renders layer options sidebar
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please describe only methods defined by us, no need to describe standard react methods

@khknopp
Copy link
Contributor Author

khknopp commented Dec 11, 2018

I deleted the more obvious functions but left out some that need explanation or are unclear and those which are particularly important.

Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kayo11 LGTM

@Ram81 Ram81 merged commit 43964c4 into Cloud-CV:master Dec 11, 2018
rahulsnkr pushed a commit to rahulsnkr/Fabrik that referenced this pull request Dec 19, 2018
* Updated UIdocumentation.md

* Update UIDocumentation.md

* Update UIDocumentation.md

* Update UIDocumentation.md

* Update UIDocumentation.md

* Update UIDocumentation.md
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

3 participants