-
Notifications
You must be signed in to change notification settings - Fork 175
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
Simulator changes (nengo_deeplearning
compatibility)
#1245
Conversation
35314a3
to
c9dba7c
Compare
@hunse I noticed something in this breaks
I believe it is this change 75db8be#diff-d4a1ea7c740725a930af968ec8143735R350 where I changed the |
It's not hard to change. One question is how significant the performance drop would be if the data isn't contiguous. I'm also not sure if that performance drop is a run-time thing or compile-time. If it's run-time, then only data that's actually not contiguous would be slower to copy. If it's compile time, then everything could be slower, because the compiler doesn't know if the data's contiguous or not, so it has to assume not. The simplest would be to make the change and run some large models (with the profiler on) and see if it makes a difference. I have a circular convolution example set up for benchmarking, so testing on that might be enough. So the short answer is it's doable, though if the change can wait, or someone else wants to do it, then that's better for me. |
It's not an urgent change at all, since I can just patch the builder within |
I moved that bit into its own PR, so the remaining changes here should all be compatible with |
@@ -125,30 +124,6 @@ def test_signal_values(): | |||
two_d.initial_value[...] = np.array([[0.5], [-0.5]]) | |||
|
|||
|
|||
def test_signal_init_values(RefSimulator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, for the next reviewer, this test is duplicated specifically by this test.
LGTM, but I'm not super confident about the 2nd commit because I have a very limited familiarity with the builder. Hopefully, the next reviewer will assuage my fears. |
nengo/utils/connection.py
Outdated
name of function object | ||
""" | ||
|
||
return getattr(func, "__name__", func.__class__.__name__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe at some point all instances of obj.__class__
were changed to type(obj)
. Would that be appropriate here too? Or is there a reason to prefer obj.__class__
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type(obj)
was what I had originally, but in Python 2.7 type(func).__name__
just displays 'instance'
, whereas func.__class__.__name__
gives you the name of the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had assumed they were the same, and if your class inherits from object
they are. For old-style classes though, the behaviour is as @drasmuss described.
Maybe I shouldn't have changed x.__class__
to type(x)
everywhere...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the commit though, I did it to help with some old types of objects. I wish I could remember what it was supposed to help with, exactly, because I agree, the name seems worse. 76f0a34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it was for MRO. For instances of old classes, x.__class__.__mro__
does not exist, whereas type(x).__mro__
does. However (and maybe I didn't realize this at the time I made the changes), type(x).__mro__
just gives (instance, object)
no matter what the actual inheritance is, so it's not useful. Does anybody know how to get the MRO of an old-style class?
Anyway, x.__class__.__name__
seems generally better to me than type(x).__name__
, so I'm fine to keep it here. We should also maybe consider reverting 76f0a34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow, missed the last bunch of comments and somewhat as side comment: I think we should in general use and encourage the usage of new-style classes (i.e. every class should inherit from object
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, and I think most people do. I would prefer not to have a hard rule about that, though, and if we do, we would want to fail right away with an explicit check, not in some random place.
Changes look good to me, except for potentially a very minor comment I made inline and as Sean not being super confident about the |
I believe that the main motivation for the no increment without set/update rule was that in the original design we didn't have our current system for setting initial values. Signals were just empty containers with no value. So the rule was supposed to check that the signal actually had a value before you tried to increment it. But we've made some changes to the signal system since then, and now all signals start with initial values, so we don't need to be as worried about that. It was a long time ago we designed that part of the builder though, so that's a bit of guess work. |
My impression was always that the set/update rule was for bug catching, but the initialization argument makes sense, too. It could have been a combination of the two. Anyway, I think we're fine without it, now. This all LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Removing PreserveValue
scares me, but let us press on in the name of progress. I added changelog entries and a commit (ea8644f) that fixed some test failures I was getting (see the commit for details).
Avoids crashing if func is a callable class, and avoids printing out the whole array if func is an array.
Removed the requirement that signals that are incremented must also be set/updated, since it doesn't seem necessary (and forces the awkwardness of PreserveValue).
Motivation and context:
This is kind of a grab bag of issues I came across while working on updates to
nengo_deeplearning
. No new features, just fixing some bugs/inconsistencies/unnecessary code. If anyone wants I can split these into separate PRs, but they're all 2--3 line changes so I thought I'd help out our PR queue and keep it all together (at least to start).How has this been tested?
Added unit tests for the bug fixes (to show that the bug is fixed). For the parts where I took out unnecessary simulator operations, the test is just that all the unit tests continue to pass with that bit removed.
How long should this take to review?
Where should a reviewer start?
Go through one commit at a time, as each is independent.
Types of changes:
Checklist: