Disable default validation for some parameters #1372

drasmuss · 2017-10-19T15:48:33Z

Some parameters rely on attributes of their parent object to perform the validation, so the validation cannot be performed until the attribute value is set on that object. Previously, attempting to set the default value for one of these parameters (e.g. net.config[nengo.Connection].transform = 2) would result in an error (because it would try to perform the validation and fail). This PR disables the validation for the defaults on those parameters. Note that the validation will still be performed when the attribute is set on an object instance (e.g., my_conn.transform = 2 or self.transform = Default).

Also adjusted some parameters that were set to non-optional with a value of None (or vice versa), which were revealed by the new test.

How has this been tested?
Added a new test, see test_params.py:test_coerce_defaults

How long should this take to review?

Average (neither quick nor lengthy)

Types of changes:

Bug fix (non-breaking change which fixes an issue)

Checklist:

I have read the CONTRIBUTING.rst document.
I have updated the documentation accordingly.
I have included a changelog entry.
I have added tests to cover my changes.
I have run the test suite locally and all tests passed.

Seanny123

LGTM. Don't forget to update the changelog before merging.

jgosmann

One general question: Why do we coerce defaults at all? If my reading of the code (in particular SupportDefaultMixin) is correct, when the actual parameter value is set from the default it is coerced anyways.

Apart from that general question and some more documentation related comments/questions, this PR looks good to me implementation-wise. It would be nice to get rid of that C901 pyflakes complexity warning for the test (it is a bit long and the interesting part is all the way to the end). But currently I cannot think of a better way to organize it either.

jgosmann · 2017-10-20T19:37:22Z

nengo/params.py

+    coerce_defaults : bool (Default: True)
+        If True, validate default values for this parameter if they are changed
+        by the config system. Setting a value on a parameter object instance
+        will always be validated.


Would it be more accurate to say 'when' instead of 'if' in the first sentence? I.e.: “If True, validate default values for this parameter when they are changed by the config system.” I would understand that as that this validation happens whenever the value is changed (by the config system), whereas the variant with 'if’ seems to imply that the validation could be separate from changing the value as long as the value is changed at all. But I'm not a native speaker, so maybe the 'if' works equally well.

In the second sentence: What does 'parameter object instance' exactly mean? Object and instance are often the same thing. Could we do with just one of the words? And what exactly is the distinction to the case described in the first sentence? Is it setting a non-default value (I assume?) or is it setting a (potentially default value) on a Parameter instance (as opposed to the Parameter class)?

This is trying to distinguish between (1) config[obj].attr = val versus (2) obj.attr = val. The user may not have any config[obj].attr = val lines in their code (i.e., they never change the defaults), in which case coerce would never be called in the (1) format and it wouldn't matter if coerce_defaults was True or False (hence the "if").

The second sentence is trying to describe (2) (obj.attr = val). It might be clearer just to take that part out, since it's describing behaviour that isn't really connected to this parameter (I was just worried that people would think there was no validation being performed if coerce_defaults=False).

Thanks, I understand now better what you intended to say. How about:

If True, validate values for this parameter when they are set in a Config object. Setting a parameter directly on an object will always be validated.

Sounds good

jgosmann · 2017-10-20T19:39:33Z

nengo/ensemble.py

-    n_neurons = IntParam('n_neurons', default=None, low=1)
-    dimensions = IntParam('dimensions', default=None, low=1)
+    n_neurons = IntParam('n_neurons', default=1, low=1)
+    dimensions = IntParam('dimensions', default=1, low=1)


These parameters aren't optional, are they? Does setting the default have any effect for non-optional parameters?

You could in theory create an ensemble like nengo.Ensemble(Default, Default), which would use the default values specified here (previously this would have tried to use None, resulting in an error). We could also make these parameters Unconfigurable if we wanted.

nengo.Ensemble(Default, Default) doesn't seem like something I would want to do or encourage. Not sure at the moment what the implications of Unconfigurable are (need to read the related code first).

I think n_neurons should not have a default value because it is not clear what a sensible default would be. 1 is the lower bound, but it is rare to actually want an ensemble with just one neuron. To me it seems better to force the user to be explicit.

I am more ok with the default dimensions=1, but would then go all the way and make the parameter optional to allow nengo.Ensemble(n_neurons).

In either case I would suggest bringing those changes up in the dev meeting if we want to go along with them. They seem somewhat fundamental. (Or just leave as it is now with default=None.)

Just to be clear, nengo.Ensemble(Default, Default) isn't a new feature introduced by this PR, that's always been possible. So there isn't any fundamentally new feature being introduced here; it was just bugged before, because the default value wasn't a valid value. So we probably shouldn't leave it as default=None.

Setting it to Unconfigurable would prevent nengo.Ensemble(Default, Default), which would be a minor change in behaviour but pretty unlikely to affect anyone. I'd be slightly inclined to leave it as a default of 1, just because it allows more functionality and there doesn't seem to be any downsides (a user can't accidentally end up confused about the default value, since they have to explicitly request it). But I don't think it matters too much either way, since this is a pretty edge use case. We can definitely talk about it more in the dev meeting if you want to add it to the agenda though 👍.

Here is my thought process: First I'm asking myself should these parameters have a default value? This comes basically down to whether there is some specific value with a special property (e.g. a transform of 1 is implementing an identity transform) or we want to say some value is a good value to use (e.g. firing rates between 200 and 400 Hz). Let us assume there is such a value for n_neurons (or dimensions), then there should not be a problem with going all the way and make the constructor argument itself optional.

But I think there is no reasonable default value for n_neurons. Yes, n_neurons=1 is special in so far as it is the lower bound, but advertising this as a good default value seems wrong to me. Yes, it would allow one to do nengo.Ensemble(Default, Default), but I don't see why someone would want to do that instead of nengo.Ensemble(1, 1) (if someone actually wants a single neuron). What I can see a use case for is something like

with nengo.Config(nengo.Ensemble) as cfg: cfg[nengo.Ensemble].n_neurons = 50 cfg[nengo.Ensemble].dimensions = 1 a = nengo.Ensemble(Default, Default) b = nengo.Ensemble(Default, Default) # ...

That, however, already works even with a default of n_neurons=None and I'm ok with that because the user still explicitely sets the number of neurons. It is a bit weird that someone has to pass in Default explicitely (I am not aware of any other situations in the API where that would be the case because usually the constructor arguments get a default of Default) and I might argue that the better way to set n_neurons on a bunch of ensemble would be:

n_neurons = 50 dimenisons = 1 a = nengo.Ensemble(n_neurons, dimensions) b = nengo.Ensemble(n_neurons, dimensions)

If this is in a subnetwork and someone wants to configure the neuron number from outside, it either needs to be passed in or it needs to be set after the creation of the network. That might seem less powerful than the Config approach, but that approach requires the subnetwork to already pass Default as argument to n_neurons which than requires the network to be created in the context of an appropriate config. It seems that explicitely requesting the number of neurons for that network as a constructor argument (to make that dependency clear) is a better idea. Hence, it seems to me that there is not much of a good use case for nengo.Ensemble(Default, Default) and we might as well set n_neurons=Unconfigurable.

Technically, setting n_neurons=Unconfigurable is a breaking change and I don't think there is anything in the current behaviour to be bothered by (it just enforces that the user sets n_neurons explicitely). So I would be fine either keeping current behaviour or making n_neurons Unconfigurable (as it is unlikely to actually affect anyone).

One final point about about setting n_neurons=1 (or to some other default), but not making it optional: To me it seems that this is hard to explain/document which I usually take as an indication that there might be some better way to structure things. Because we were to set a default value, we ought to document it in the docstring, but at the same time function arguments with a default are also optional in Python. So I am not sure how to document this behaviour that there is a default value, but it only comes into play in this special circumstance where the function argument is set explicitely to the Default.

tl;dr:

I am not convinced that there is a good use case for nengo.Ensemble(Default, Default).

I do not think that there is a good default value for n_neurons.

If we set a default anyways, we should also make the parameter optional.

I'm fine with either keeping current behaviour or change the default to Unconfigurable (technically breaking, but unlikely to affect anyone).

jgosmann · 2017-10-20T20:01:56Z

nengo/tests/test_params.py

@@ -322,3 +323,63 @@ class Test(object):
        obsolete = params.ObsoleteParam('obsolete', 'not included in params')

    assert set(params.iter_params(Test())) == {'p1', 'p2'}
+
+
+def test_coerce_defaults():  # noqa: C901


I think we might potentially be able to come up with a better name for this test. As far as I can tell it is not really testing the correct coercion of defaults, but rather that setting values in a config does not fail if the coercion cannot be done at that point (because missing information about parent objects).

No strong opinions, feel free to change it to whatever you'd like.

drasmuss · 2017-10-20T20:43:12Z

One general question: Why do we coerce defaults at all? If my reading of the code (in particular SupportDefaultMixin) is correct, when the actual parameter value is set from the default it is coerced anyways.

The idea is to give an informative error message earlier on if the user has set the wrong value. For example, if the user does

with nengo.Network():
    net.config[nengo.Ensemble].eval_points = "abc"  # line 1
    ....
    a = nengo.Ensemble(10, 1)  # line 2

If we don't check the defaults, then the user will get an error at line 2 saying that they're trying to create an Ensemble with invalid eval points, which will be somewhat confusing and difficult to track down since they are not setting the eval points on that ensemble. It's nicer if they get an error at line 1 telling them that the new default value they've set is invalid.

tbekolay · 2017-10-20T20:48:46Z

For some additional context, Dan and I also discussed the possibility of having a separate method for coercing defaults, which would do some basic validation but nothing that relies on the object instance. It seemed like too much code for too little benefit, so we decided against it.

drasmuss · 2017-10-23T20:18:13Z

Added a commit to make n_neurons/dimensions unconfigurable.

jgosmann

Some more small fixes. LGTM now. 🍰

drasmuss · 2017-10-23T21:00:32Z

nengo/params.py

@@ -445,7 +445,7 @@ def coerce_ndarray(self, instance, ndarray):  # noqa: C901

    @property
    def coerce_defaults(self):
-        return not ("n_neurons" in self.shape or "dimensions" in self.shape)
+        return not any(is_string(dim) for dim in self.shape)


"..." and "*" are valid shape strings that don't need to disable the default coercion

right, fixed it

drasmuss · 2017-10-23T21:02:18Z

CHANGES.rst

@@ -26,7 +26,7 @@ Release History

 - Default values can no longer be set for ``Ensemble.n_neurons`` or
  ``Ensemble.dimensions``
-  (`#1372 <https://github.com/nengo/nengo/pull/1372>`__)
+  (`#1372 <https://github.com/nengo/nengo/pull/1372>`_)


This needs to be an anonymous hyperlink, since it is identical to the one below.

my bad, didn't know about anonymous hyperlinks, also fixed.

Seanny123

LGTM. flake8 checks are failing, but only due to problems not associated to this PR.

tbekolay

Made a commit with some minor things, mostly fixing the complexity warning with a dict. Will merge after lunch if no objections!

jgosmann · 2017-11-02T16:11:51Z

nengo/tests/test_params.py

    with nengo.Network() as net:
+        # NB: Probe must go before Connection


What does NB stand for?
Why must the Probe go before connection?
What do you think about using pytest.mark.parametrize to eliminate the outer for loop?

btw: I like the dictionary approach. :)

NB basically mean important note, thought it was relatively common but I can change it if it's not.

Probe.solver uses connection's default by default. I had changed the for loop to use net.objects (which has all the types) instead of the list and got failures because the probe solver was using weights; this was because the connection default had changed, and the probe solver uses the connection's default by default. I added the note so that other people wouldn't try to do the same and get failures due to dict ordering.

IMO parametrize would inflate the number of tests by a fair bit, and doesn't feel like it'd be any more readable, so I would opt for keeping it as is.

Probe.solver uses connection's default by default. I had changed the for loop to use net.objects (which has all the types) instead of the list and got failures because the probe solver was using weights; this was because the connection default had changed, and the probe solver uses the connection's default by default. I added the note so that other people wouldn't try to do the same and get failures due to dict ordering.

Shouldn't that be solved by moving the with nengo.Network() as net: inside the for loop? That seems better to me because it isolates the indivdual tests and the order of iteration does not matter. (If I'm not overlooking something.)

IMO parametrize would inflate the number of tests by a fair bit, and doesn't feel like it'd be any more readable, so I would opt for keeping it as is.

The output for a test failure might be more readable, but I would have to actually try that out and not sure if I'm feeling that strongly about it ...

Shouldn't that be solved by moving the with nengo.Network() as net: inside the for loop?

I don't think that would matter, as you're always within that network's context, so if the connection default is set before the probe default, it will use it.

You could get around it by simply not being in the network's context though; it's not actually necessary for anything the test is doing. Just tested it and it works, so I'll push that change.

tbekolay · 2017-11-02T18:11:45Z

The appveyor failure seems to be outside our control at the moment, and the TravisCI build will be fixed once rebased. OK if I merge @jgosmann ?

jgosmann · 2017-11-02T18:18:40Z

👍 Go ahead.

Some parameters rely on attributes of their parent object to perform the validation, so the validation cannot be performed until the attribute value is set on that object. Additionally, Ensemble.n_neurons and Ensemble.dimensions no longer have a default value set, meaning that they cannot be configured to have a default.

Seanny123 approved these changes Oct 20, 2017

View reviewed changes

jgosmann reviewed Oct 20, 2017

View reviewed changes

jgosmann approved these changes Oct 23, 2017

View reviewed changes

drasmuss commented Oct 23, 2017

View reviewed changes

Seanny123 approved these changes Oct 25, 2017

View reviewed changes

tbekolay approved these changes Nov 2, 2017

View reviewed changes

jgosmann reviewed Nov 2, 2017

View reviewed changes

tbekolay force-pushed the coerce_defaults branch from a66aa98 to fb8f5e8 Compare November 2, 2017 18:37

tbekolay merged commit fb8f5e8 into master Nov 2, 2017

tbekolay deleted the coerce_defaults branch November 2, 2017 18:56

jgosmann mentioned this pull request Jun 27, 2019

Cannot change n_neurons post ensemble creation #1547

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable default validation for some parameters #1372

Disable default validation for some parameters #1372

drasmuss commented Oct 19, 2017

Seanny123 left a comment

jgosmann left a comment

jgosmann Oct 20, 2017

drasmuss Oct 20, 2017

jgosmann Oct 20, 2017

drasmuss Oct 21, 2017

jgosmann Oct 20, 2017

drasmuss Oct 20, 2017

jgosmann Oct 20, 2017

drasmuss Oct 21, 2017

jgosmann Oct 23, 2017

jgosmann Oct 20, 2017

drasmuss Oct 20, 2017

drasmuss commented Oct 20, 2017

tbekolay commented Oct 20, 2017

drasmuss commented Oct 23, 2017

jgosmann left a comment

drasmuss Oct 23, 2017

jgosmann Oct 23, 2017

drasmuss Oct 23, 2017

jgosmann Oct 23, 2017

Seanny123 left a comment

tbekolay left a comment

jgosmann Nov 2, 2017

jgosmann Nov 2, 2017

tbekolay Nov 2, 2017 •

edited

Loading

jgosmann Nov 2, 2017

tbekolay Nov 2, 2017

tbekolay commented Nov 2, 2017

jgosmann commented Nov 2, 2017

		with nengo.Network() as net:
		# NB: Probe must go before Connection

Disable default validation for some parameters #1372

Disable default validation for some parameters #1372

Conversation

drasmuss commented Oct 19, 2017

Seanny123 left a comment

Choose a reason for hiding this comment

jgosmann 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

Choose a reason for hiding this comment

drasmuss commented Oct 20, 2017

tbekolay commented Oct 20, 2017

drasmuss commented Oct 23, 2017

jgosmann 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

Seanny123 left a comment

Choose a reason for hiding this comment

tbekolay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbekolay Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbekolay commented Nov 2, 2017

jgosmann commented Nov 2, 2017

tbekolay Nov 2, 2017 •

edited

Loading