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

Neural Network widget #2553

Merged
merged 3 commits into from
Sep 19, 2017
Merged

Neural Network widget #2553

merged 3 commits into from
Sep 19, 2017

Conversation

ajdapretnar
Copy link
Contributor

@ajdapretnar ajdapretnar commented Aug 29, 2017

Issue

Neural Networks only in scripting part, not in a widget.

Description of changes

Added a simple, cumbersome NN widget. Should work for basic tasks. Documentation pending until layout consolidated.

Includes
  • Code changes
  • Tests
  • Documentation


class OWNNLearner(OWBaseLearner):
name = "Neural Network"
description = "Predict according to the nearest training instances."
Copy link
Contributor

Choose a reason for hiding this comment

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

Update description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ups.

callback=self.settings_changed)
self.activation_combo = gui.comboBox(
box, self, "activation_index", orientation=Qt.Horizontal,
label="Activation:", items=[i.capitalize() for i in self.activation],
Copy link
Contributor

Choose a reason for hiding this comment

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

The correct human-friendly labels would be: Identity, Logistic, tanh and ReLU. Imho .upper() would server better.

callback=self.settings_changed)
self.solver_combo = gui.comboBox(
box, self, "solver_index", orientation=Qt.Horizontal,
label="Solver:", items=[i.capitalize() for i in self.solver],
Copy link
Contributor

Choose a reason for hiding this comment

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

.upper() would server better. Those are acronyms.

"hidden_layers_input",
label="Hidden layers:",
orientation=Qt.Horizontal,
callback=self.settings_changed)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tooltip= describing its format.

box = gui.vBox(self.controlArea, "Network")
self.hidden_layers_edit = gui.lineEdit(box, self,
"hidden_layers_input",
label="Hidden layers:",
Copy link
Contributor

Choose a reason for hiding this comment

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

Neurons per hidden layer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neurons and no of layers. This was a big pain and this is a sad sad cheat to accommodate for n layers of m neurons. It is a workaround to avoid complex GUI with +/- for layers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll update the label.

@ajdapretnar
Copy link
Contributor Author

@kernc Would you care to check whether spin box maxval and steps make sense or not? To adjust this as well, while I'm at it.

@kernc
Copy link
Contributor

kernc commented Aug 29, 2017

I'd never wait to scroll through [1e-5, 1] in 1e-5 increments. Perhaps 1e-2 stepsize makes more sense in general, but the majority of users will just edit the number manually.

@codecov-io
Copy link

codecov-io commented Aug 30, 2017

Codecov Report

Merging #2553 into master will increase coverage by <.01%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master    #2553      +/-   ##
==========================================
+ Coverage   75.08%   75.09%   +<.01%     
==========================================
  Files         327      329       +2     
  Lines       57725    57773      +48     
==========================================
+ Hits        43341    43382      +41     
- Misses      14384    14391       +7

@ajdapretnar
Copy link
Contributor Author

All updated.

callback=self.settings_changed,
tooltip="A list of integers "
"defining neurons. "
"Lenght of list "
Copy link
Contributor

@kernc kernc Aug 30, 2017

Choose a reason for hiding this comment

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

typo. Also might be better to indent these params a single level, like the rest below.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

from Orange.evaluation import CA, CrossValidation, MSE


class TestKNNLearner(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why TestKNNLearner?

Busted!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Busted!

L33.717,27.185z"/>
<path fill="#989898" d="M28.403,24.722l4.067-0.016l0.084,1.769l3.396-2.431l-3.612-2.101l0.084,1.762l-3.985,0.016
C28.464,24.05,28.462,24.383,28.403,24.722z"/>
</svg>
Copy link
Contributor

Choose a reason for hiding this comment

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

Icon is kind of crowded, lines are too thin... Maybe remove some arrows, and reduce the right-most layer to two neurons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Icon is taken from Orange2 and will be replaced with something nicer from Lara. Soon. :)

callback=self.settings_changed,
tooltip="A list of integers "
"defining neurons. "
"Lenght of list "
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

"layers. E.g. 4, 2, 2, 3.")
self.activation_combo = gui.comboBox(
box, self, "activation_index", orientation=Qt.Horizontal,
label="Activation:", items=[i.upper() for i in self.activation],
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use upper. It looks ugly. If you'd like some -- but not all -- letters in upper case, add a list of labels in parallel with the method names you have now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, ok.

box, self, "alpha", 1e-5, 1.0, 1e-2,
label="Alpha:", decimals=5, alignment=Qt.AlignRight,
callback=self.settings_changed, controlWidth=80)
self.max_iter_spin = gui.spin(
Copy link
Contributor

Choose a reason for hiding this comment

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

Right-align.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean Qt.AlignRight as parameter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Umm, yes add, alignment=Qt.AlignRight to the last spinbox. Numbers usually look better if aligned to the right. (Neurons per hidden layer is different.)

def get_learner_parameters(self):
return (("Hidden layers", self.get_hidden_layers()),
("Activation", self.activation[self.activation_index].capitalize()),
("Solver", self.solver[self.solver_index].capitalize()),
Copy link
Contributor

Choose a reason for hiding this comment

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

capitalize or upper or a list of user-readable names.

preprocessors=self.preprocessors)

def get_learner_parameters(self):
return (("Hidden layers", self.get_hidden_layers()),
Copy link
Contributor

Choose a reason for hiding this comment

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

This prints out as: Hidden layers: (100,)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It should. I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes, I see what's the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no, I don't.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like hidden_layer_sizes=", ".join(map(str, self.get_hidden_layers()))

Copy link
Contributor

@janezd janezd left a comment

Choose a reason for hiding this comment

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

Perhaps you can set the default layer size to empty string, hidden_layers_input = Setting(""), and add
placeholderText="e.g. 100, 200, 100" to the arguments of hidden_layers_edit. This will show this "tooltip" to the user.

Then, of course, remove self.hidden_layers_edit.setText("100") in get_hidden_layers.

The problem with this is that the user doesn't know then that the default is 100. Maybe

        self.hidden_layers_edit = gui.lineEdit(
            box, self, "hidden_layers_input",
            label="Neurons per hidden layers:",
            placeholderText="e.g. 100, 200, 100; default is 100",
            orientation=Qt.Vertical,
            callback=self.settings_changed,
            addSpace=24)

box, self, "alpha", 1e-5, 1.0, 1e-2,
label="Alpha:", decimals=5, alignment=Qt.AlignRight,
callback=self.settings_changed, controlWidth=80)
self.max_iter_spin = gui.spin(
Copy link
Contributor

Choose a reason for hiding this comment

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

Umm, yes add, alignment=Qt.AlignRight to the last spinbox. Numbers usually look better if aligned to the right. (Neurons per hidden layer is different.)

self.hidden_layers_edit = gui.lineEdit(box, self,
"hidden_layers_input",
label="Neurons per hidden "
"layer:",
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps layer -> layers? It is not obvious the user can input a list. Not many users will find the tooltip.

preprocessors=self.preprocessors)

def get_learner_parameters(self):
return (("Hidden layers", self.get_hidden_layers()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Something like hidden_layer_sizes=", ".join(map(str, self.get_hidden_layers()))

@ajdapretnar ajdapretnar force-pushed the nn-widget branch 2 times, most recently from 4a45b0d to a66422d Compare September 4, 2017 09:15
@ajdapretnar
Copy link
Contributor Author

I don't think the widget should have an empty string as a default, especially since one can't use it on the fly. All other widgets have sklearn's default values and so should this one.
Moreover, the user can see the default is "100". Not sure, perhaps I can set the default to "100,". Would this make more sense?

Everything else is fixed, once agreed I can add docs and call it a day. ;)

@kernc
Copy link
Contributor

kernc commented Sep 4, 2017

An empty string results in no hidden layers. Such a perceptron is not forbidden by sklearn.

Set the default to "100,", but also set the placeholder?

("Max iterations", self.max_iterations))

def get_hidden_layers(self):
layers = tuple(map(int, re.findall('\d+', self.hidden_layers_input)))
Copy link
Member

Choose a reason for hiding this comment

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

Lint suggests you use r'\d+'. I agree

@ajdapretnar
Copy link
Contributor Author

Fixed and rebased.

@ajdapretnar
Copy link
Contributor Author

I don't understand why tests don't pass. They pass locally.

@astaric
Copy link
Member

astaric commented Sep 14, 2017

It's not the tests that are failing. If you click on details, you can see that the pylint and documentation builds have failed.

Pylint has two complaints:

  1. When you do MSE(), you get a scoring function, when the scoring function is given results, it returns a score. The way you should call it is MSE()(results) (or scorer = MSE(); scorer(results))
  2. It wants QApplication import at the top of the file.

Documentation complains about the images in the documentation not being as small as they could be.

@ajdapretnar
Copy link
Contributor Author

I guess it's fixed now.

@ajdapretnar
Copy link
Contributor Author

I'm eager to merge this. Please advise what to do with the rouge import line in __main__?

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.

5 participants