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

Added 'NoSolver' solver #1352

Merged
merged 1 commit into from
Sep 21, 2017
Merged

Added 'NoSolver' solver #1352

merged 1 commit into from
Sep 21, 2017

Conversation

bmorcos
Copy link
Contributor

@bmorcos bmorcos commented Sep 6, 2017

Motivation and context:
It has come up a few times that it would be nice to set decoders or weights directly. More specifically, loading from previous runs as mentioned in #649, #608. This doesn't facilitate saving and loading per se, but does allow the user to set decoders or weights even if they do not have access to pre.neurons. This can be used to help load and save decoders or weights as outlined in nengo/nengo-extras#35.

How has this been tested?
Added a new test, all current tests passed.

How long should this take to review?

  • Quick (less than 40 lines changed or changes are straightforward)

Where should a reviewer start?
Just look at the commit.

Types of changes:

  • New feature (non-breaking change which adds functionality)

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.
  • All new and existing tests passed.

Still to do:

  • Add some error checking on values to give better error messages for shape mismatch

@bmorcos
Copy link
Contributor Author

bmorcos commented Sep 6, 2017

I'm wondering if we want to add warnings on high values too maybe?

@arvoelke
Copy link
Contributor

arvoelke commented Sep 6, 2017

Yay! I've needed this before, as has @psipeter.

But, I am wondering about the name PassThrough. In signal processing, passthrough has a pretty specific meaning. For instance, the D matrix in a state-space model (linear system) is often called the passthrough matrix [1, p. 427], and I use this terminology in nengolib.

Might I suggest a name like StaticWeights, FixedWeights, or PreSolvedWeights? (Edit: maybe not the first, since the connection could have learning enabled)

Also, note that the variable self.value will change from None before building to something else afterward. This may be unexpected if the user (for whatever reason) is accessing conn.solver.value both before build and after build. Most objects in Nengo appear to be frozen (immutable) unless there's some need otherwise. More importantly, this will cause a bug if the same solver instance is used for multiple connections. It's certainly not common to see, but it's technically legal to create a single solver instance (solver=Passthrough()) and then pass this same solver to multiple connections (in fact this is what happens underneath when you use the config system to override the solver for an entire subnetwork). Currently, your solver would change self.value when the first connection is solved, providing a shape that will not necessarily match the A (or Y or E) needed for the second connection. Removing the mutation should fix both issues.

[1] Seeler, K.A. System Dynamics: An Introduction for Mechanical Engineers. Springer New York, 2014.

@Seanny123
Copy link
Contributor

At the risk of bike-shedding, how about InitialWeights instead of PreSolvedWeights?

@bmorcos
Copy link
Contributor Author

bmorcos commented Sep 7, 2017

I think I like InitialWeights as these aren't necessarily fixed/static.

Most objects in Nengo appear to be frozen (immutable) unless there's some need otherwise.

We don't have access to n_neurons, dimensions, or post.n_neurons until __call__ which dictates the shape. I thought it would be nice to default to zeros for empty args, but we don't have to do this.

Currently, your solver would change self.value when the first connection is solved, providing a shape that will not necessarily match the A (or Y or E) needed for the second connection

This is a tricky one I think... if we are using None to default to zeros, then it would be nice to have one instance create all the correct shapes. For the broader use of setting weights, you couldn't use the same instance of the solver unless the connection shape was identical anyways. Maybe we just add an error or warning about this?

@tbekolay
Copy link
Member

tbekolay commented Sep 7, 2017

This is a hard one to name for sure... one other possibility to throw out there is NoSolver, since one important aspect of this class is that no decoder solving will be happening. I think I prefer that to IntialWeights, which is confusing in the context of nengo.Connection; e.g.:

nengo.Connection(pre, post, solver=InitialWeights(myweights))
nengo.Connection(pre, post, solver=NoSolver(myweights))

I also think Passthrough makes some sense in the nengo.Connection context too, but NoSolver is probably better. But yeah, in general I think that a term having a definition in some other context is a good thing (and not a dealbreaker) as that general idea can translate to this context (as long as it's not confusing, which I don't think it is in this case).

I thought it would be nice to default to zeros for empty args, but we don't have to do this.

I think the comment about immutability can be easily resolved by returning all zeros without setting self.value. There's no reason to change self.value in the __call__ method; all that matters is what is returned.

@bmorcos
Copy link
Contributor Author

bmorcos commented Sep 7, 2017

That commit addresses a couple points, I think we still need to talk about error checking here though. if there is a shape mismatch the current error is not very obvious:

  File "<stdin>", line 1, in <module>
  File "/home/ben/Git/nengo/nengo/simulator.py", line 156, in __init__
    self.model.build(network, progress_bar=self.progress_bar)
  File "/home/ben/Git/nengo/nengo/builder/builder.py", line 123, in build
    built = self.builder.build(self, obj, *args, **kwargs)
  File "/home/ben/Git/nengo/nengo/builder/builder.py", line 218, in build
    return cls.builders[obj_cls](model, obj, *args, **kwargs)
  File "/home/ben/Git/nengo/nengo/builder/network.py", line 118, in build_network
    model.build(conn)
  File "/home/ben/Git/nengo/nengo/builder/builder.py", line 123, in build
    built = self.builder.build(self, obj, *args, **kwargs)
  File "/home/ben/Git/nengo/nengo/builder/builder.py", line 218, in build
    return cls.builders[obj_cls](model, obj, *args, **kwargs)
  File "/home/ben/Git/nengo/nengo/builder/connection.py", line 264, in build_connection
    tag="%s.weights_elementwiseinc" % conn))
  File "/home/ben/Git/nengo/nengo/builder/operator.py", line 554, in __init__
    A.initial_value, X.initial_value, Y.initial_value, self.tag)
  File "/home/ben/Git/nengo/nengo/builder/operator.py", line 499, in reshape_dot
    % (tag, A.shape, X.shape, Y.shape))
nengo.exceptions.BuildError: shape mismatch in <Connection from <Ensemble (unlabeled) at 0x7f997cc1fc50> to <Ensemble (unlabeled) at 0x7f997cc2a908>>.weights_elementwiseinc: (2, 2) x (10,) -> (2,)

To reiterate again what @arvoelke brought up, we might want to at least warn if the same solver instance is used multiple times?

@arvoelke
Copy link
Contributor

arvoelke commented Sep 7, 2017

To reiterate again what @arvoelke brought up, we might want to at least warn if the same solver instance is used multiple times?

Your changes at a glance look to fix this now, so a warning shouldn't be necessary. A legitimate use-case would be using the config system to set initial weights to zero for each ensemble in an EnsembleArray, and from what I can see there is no longer a potential issue here (perhaps a unit test is in order?). 👍 There may still be some more error checking needed for the case when values are supplied however. Haven't given this thought.

@bmorcos bmorcos changed the title Added 'PassThrough' solver Added 'NoSolver' solver Sep 8, 2017
Copy link
Member

@tbekolay tbekolay left a comment

Choose a reason for hiding this comment

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

Looks good to me! I made a few minor changes, added the solver to the documentation, and added a changelog entry in some new commits.

Used to manually pass in weights. Useful as an alternative
to passing in a transform.
@tbekolay tbekolay merged commit 1262b8c into master Sep 21, 2017
@tbekolay tbekolay deleted the passthrough branch September 21, 2017 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants