Update owneuralnetwork.py to handle initial random state for ANN learner #3715

WolframRinke · Apr 1, 2019

Added the option to preset the initail random value. Extra input field in the form.
0 = randomizer behavior like before

0 ranomizer is initiated with the defined value.

Issue

Description of changes

Includes

Code changes
Tests
Documentation

CLAassistant · Apr 1, 2019

All committers have signed the CLA.

codecov · Apr 1, 2019

Codecov Report

Merging #3715 into master will decrease coverage by <.01%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #3715      +/-   ##
==========================================
- Coverage   84.52%   84.52%   -0.01%     
==========================================
  Files         373      373              
  Lines       68459    68465       +6     
==========================================
+ Hits        57868    57873       +5     
- Misses      10591    10592       +1

janezd · Apr 5, 2019

Thanks for this pull request.

We had a discussion at our weekly meeting. You're right about the issue, allowing the user to choose between replicable or random behavior is a good idea. But we don't like allowing the user to set a (favorable? :)) seed. Somebody mentioned another widget (random forest) that allows it, so we said we'll remove it from there, too.

So we agreed to have a checkbox "Replicable training" (if anybody has a better phrase, welcome), but without letting the user manipulate the seed. This will thus be the same as in Data Sampler widget.

I'm still going to add a few comments, as guidelines for the next time you (hopefully) decide to contribute.

janezd · Apr 5, 2019

janezd reviewed Apr 5, 2019

View changes

janezd · Apr 5, 2019

Orange/widgets/model/owneuralnetwork.py

@@ -83,6 +83,7 @@ class OWNNLearner(OWBaseLearner):
    max_iterations = Setting(200)
    alpha_index = Setting(0)
    settings_version = 1
+    random_state = 1


This should be random_state = Setting(1) so the widget will remember this setting. Also, it should be above settings_version = 1 -- because it's a setting, while settings_version has a number that is increased when settings are changed in such a way that they need to be migrated.

janezd · Apr 5, 2019

Orange/widgets/model/owneuralnetwork.py

@@ -135,6 +136,14 @@ def add_main_layout(self):
                None, self, "max_iterations", 10, 10000, step=10,
                label="Max iterations:", orientation=Qt.Horizontal,
                alignment=Qt.AlignRight, callback=self.settings_changed))
+
+        form.addRow(
+            "Random state:",


We, programmers, know what "Random state" this means, but it may not be understood by muggles. We try to explain these in non-technical terms -- by what it means (replicability...) and not how it's implemented (fixed initial random state).

Its is not necessary to enter an initial value as long there is a way to fix it or use the same start value each time. So probalby a checkbox might do the same.

I think @janezd was referring to the naming of the checkbox. It should say something like 'Replicable', the way it is done in Data Sampler.

I agree. 'Replicable' is fine for me.

BTW: Is there any further action expected from my side, as the status has been closed?
Shall I open another pullrequest with the code changes or will this be implemented by someone else?

Is there any further action expected from my side

I opened #3724. I suppose it will be implemented on Friday.

However, you can do it if you wish. In this case, consider the comments that I've written as guidelines. Including removing redundant parentheses in conditions. :)

Ok, great.
So I will consider your suggestions and do the changes and test it within my application.

janezd · Apr 5, 2019

Orange/widgets/model/owneuralnetwork.py

+        form.addRow(
+            "Random state:",
+            gui.spin(
+                None, self, "random_state", 0, 100000, step=20,


What does step=20 accomplish? Is the difference between 10 and 30 larger than between 10 and 11? :)

No issue, it can be what ever value you like. ;-)
So a step size of 1 is fine, but if we drop direct entry it does not make sense anyway.

janezd · Apr 5, 2019

Orange/widgets/model/owneuralnetwork.py

+                alpha=self.alpha,
+                max_iter=self.max_iterations,
+                preprocessors=self.preprocessors)
+        return learner


These two calls are the same, except that the second uses the default values for random_state. You should have a single call with a variable parameter.

Exactly. This is the reason for the conditional statement here. As I don't see a way to access the random state from the underlying library, the easiest way is to have two different calls. One with the fixed random.state and the other without it. Or you fix it in the random.state setup. For me this is more clear, than messing with hidden object states.

You pass the value of self.random_state here (probably 0) or None if the checkbox is off.

janezd · Apr 5, 2019

Orange/widgets/model/owneuralnetwork.py

-            max_iter=self.max_iterations,
-            preprocessors=self.preprocessors)
+        learner = None
+        if (self.random_state > 0):


No parentheses here in Python.

Makes it more readable to me without any drawbacks.

janezd · Apr 5, 2019

Orange/widgets/model/owneuralnetwork.py

-            alpha=self.alpha,
-            max_iter=self.max_iterations,
-            preprocessors=self.preprocessors)
+        learner = None


This is redundant; learner will be set to something in one of the following lines.

Than we can drop it.

WolframRinke · Apr 7, 2019

Thank you for considering this feature.

janezd added the needs discussion label Apr 4, 2019

janezd assigned markotoplak, BlazZupan and janezd Apr 4, 2019

janezd removed the needs discussion label Apr 5, 2019

janezd unassigned markotoplak and BlazZupan Apr 5, 2019

janezd closed this Apr 5, 2019

janezd referenced this pull request Apr 5, 2019
Open
Replicability in Neural networks and Random forests #3724

biolab/orange3

Update owneuralnetwork.py to handle initial random state for ANN learner #3715

Update owneuralnetwork.py to handle initial random state for ANN learner #3715

WolframRinke commented Apr 1, 2019

This comment has been minimized.

CLAassistant commented Apr 1, 2019 •

edited

This comment has been minimized.

codecov bot commented Apr 1, 2019

janezd added the needs discussion label Apr 4, 2019

janezd assigned markotoplak, BlazZupan and janezd Apr 4, 2019

janezd removed the needs discussion label Apr 5, 2019

janezd unassigned markotoplak and BlazZupan Apr 5, 2019

This comment has been minimized.

janezd commented Apr 5, 2019

janezd closed this Apr 5, 2019

janezd reviewed Apr 5, 2019

View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

janezd referenced this pull request Apr 5, 2019

Replicability in Neural networks and Random forests #3724

This comment has been minimized.

WolframRinke commented Apr 7, 2019

biolab/orange3

Join GitHub today

Update owneuralnetwork.py to handle initial random state for ANN learner #3715

Conversation

WolframRinke commented Apr 1, 2019

Issue

Description of changes

Includes

This comment has been minimized.

CLAassistant commented Apr 1, 2019 • edited

This comment has been minimized.

codecov bot commented Apr 1, 2019

Codecov Report

janezd added the needs discussion label Apr 4, 2019

janezd assigned markotoplak, BlazZupan and janezd Apr 4, 2019

janezd removed the needs discussion label Apr 5, 2019

janezd unassigned markotoplak and BlazZupan Apr 5, 2019

This comment has been minimized.

janezd commented Apr 5, 2019

janezd closed this Apr 5, 2019

janezd reviewed Apr 5, 2019 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

janezd referenced this pull request Apr 5, 2019

This comment has been minimized.

WolframRinke commented Apr 7, 2019

CLAassistant commented Apr 1, 2019 •

edited

janezd reviewed Apr 5, 2019

View changes