Warn if gain, bias override intercepts, max_rates #1433
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Seanny123
approved these changes
Apr 24, 2018
jgosmann
reviewed
Apr 24, 2018
nengo/builder/ensemble.py
Outdated
| warnings.warn( | ||
| "Specifying the gains and biases for %s imposes a set of " | ||
| "maximum firing rates and intercepts. Further specifying " | ||
| "either max_rates or intercepts has no effect." % ens) |
There was a problem hiding this comment.
I would make this a NengoWarning.
There was a problem hiding this comment.
When initially thinking about this, I stuck with a UserWarning for consistency with the warning issued by gen_eval_points. In any event, I've added a fixup to use a NengoWarning, but it doesn't seem like there is a widespread usage of these throughout the codebase (i.e. most warnings are UserWarnings based on a quick search). Is there a rule of thumb for when one should preferred over the other? Or should most of the existing UserWarnings be switched over to NengoWarnings? (I'm happy to do this at some point if that is the case)
jgosmann
approved these changes
Apr 24, 2018
tbekolay
approved these changes
Apr 24, 2018
tbekolay
force-pushed
the
ensemble_build_warning
branch
from
541b17e
to
dbcecb5
Compare
tbekolay
force-pushed
the
ensemble_build_warning
branch
from
dbcecb5
to
eb9cafc
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and context:
See issue #1431. This PR adds a warning that occurs whenever an ensemble is built for which specified gains and biases override specified intercepts and/or max_rates. I'm happy to get feedback regarding, for example, whether it makes sense to either issue a warning elsewhere, or add something to the documentation that explains how gains, biases, max_rates, and intercepts are related, and then count on the user not to create ensembles of the sort under consideration here.
Interactions with other PRs:
None so far as I know.
How has this been tested?
I've added a test that checks whether a
UserWarningis issued whenever an ensemble is built for which gains, biases and at least one of max_rates and intercepts are specified.How long should this take to review?
Where should a reviewer start?
The
get_gain_biasfunction innengo/builder/ensemble.pyhas the core change, so it's likely best to start there before moving on to looking at the corresponding test.Types of changes:
Checklist:
There's no docstring for the
get_gain_biasfunction, and it is likely not something that a user would ever call directly, so I haven't added any documentation. It also seems like a small enough change to avoid including a changelog entry, but I'm happy add this if it makes sense to.Still to do:
Nothing at this point.