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

Updates for recent Nengo changes #128

Merged
merged 1 commit into from
Nov 29, 2016
Merged

Updates for recent Nengo changes #128

merged 1 commit into from
Nov 29, 2016

Conversation

tbekolay
Copy link
Member

We're hoping to push out a Nengo 2.3.0 release soon, so I made some changes to fix compatibility as a result of some recent changes, specifically nengo/nengo#1151 and nengo/nengo#1173.

With these commits, the tests run successfully for me with Nengo master (soon to be 2.3.0) and with the versions that previously worked (e.g. 2.1.2.).

@@ -431,6 +431,15 @@ def __init__(self, network, dt=0.001, seed=None, model=None, context=None,
self._reset_cl_rngs()
self._probe_step_time()

@property
def model(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this inherited from nengo.Simulator?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is in Nengo master, but when using OCL with older versions of Nengo it doesn't exist, so this is here just for compatibility.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, right, but then people will get warnings if they call nengo_ocl.Simulator.model with an older version of Nengo.

@hunse
Copy link
Collaborator

hunse commented Nov 15, 2016

I don't like what happened in nengo/nengo#1173. To me it seems too prescriptive. If a user wants to access sim.model, why not? I do it sometimes to look into individual signals and see what's going on. It's not something I would advertise, but I'd prefer not to have a warning. In general, I prefer the carrot to the stick, and would rather just have good examples of using Simulator.data to access stuff, rather than needing a warning when users access sim.model. Anyway, that ship has sailed already.

So the question is for here, what to do. I was happy with my self.model everywhere in the code, and now I have to put underscores! (I'm partly joking, but it is uglier.) One thing we could do is override Simulator.model to not have the warning and just return self._model. But it's also not a huge change, and there are benefits to being consistent with nengo, too. But I can definitely see myself using self.model somewhere in this simulator, and not finding it because I don't think py.test shows warnings by default. I'm torn.

@tbekolay
Copy link
Member Author

I don't like what happened in nengo/nengo#1173.

That PR's in master but it hasn't been released, so I would say the ship hasn't necessarily sailed. @drasmuss was also in favor of reverting it, so that's an alternative to the changes in this PR. I don't feel too strongly either way, but I agree that it does seem somewhat prescriptive. Actually, I was somewhat surprised that people were using sim.model.params for anything since examples and test only ever use sim.data, but clearly it was being used. The question is whether that's a problem, and I think I agree that it's not really a problem. The ugliness of _model is not lost on me (you'll see very few underscores in things I've written).

One thing we could do is override Simulator.model to not have the warning and just return self._model

I actually originally did this originally, but nengo/nengo#1173 also included a test that accessing sim.model raises a warning so I included the warning.


Anyhow, since there it's contentious I'll open up a vote on the forum and point people it at the next dev meeting.

@jgosmann
Copy link
Contributor

If a user wants to access sim.model, why not?

Sure, but the user should be aware that they are accessing an implementation detail, that might not be supported by all backends or might change without notice. That is why I think there should be an underscore in front.

It's not something I would advertise, but I'd prefer not to have a warning.

You don't get a warning when using sim._model or you can silence the warning.

good examples of using Simulator.data to access stuff

I was somewhat surprised that people were using sim.model.params for anything since examples and test only ever use sim.data

I don't think we have any examples of accessing static parameters generated during the build (but I might be wrong) and I don't think there is any real documentation so far how to do that. Thus, it was not obvious that sim.model.params is copied to sim.data.

Also, there was a number of tests using sim.model.params prior to merging nengo/nengo#1173. (Thus, please don't revert all of nengo/nengo#1173, but only the renaming of model to _model.)

One thing we could do is override Simulator.model to not have the warning and just return self._model

You mean only in Nengo OCL? (Thus, the reference simulator still gives the warning.) I might not endorse it, but would be ok with that. Though, keep in mind that the underscore is a common Python idiom.

not finding it because I don't think py.test shows warnings by default

You could configure that warning to raise an exception.

The question is whether that's a problem, and I think I agree that it's not really a problem.

It violates the Zen of Python: “There should be one-- and preferably only one --obvious way to do it.”
Certainly not an absoltue law, but something I would consider.


With nengo/nengo#1173 I did not really intend to prescribe this warning to other backends. As mentioned before I think the main problem here is nengo_ocl.Simulator inheriting from nengo.Simulator. This obviously won't get fixed in the short term, but given that it should be fixed in the future it is ok to introduce some slight “ugliness” in Nengo OCL for now rather than removing guidance what the public interface is from core Nengo.

This changed in Nengo 2.3. In order to maintain compatibility with
older versions of Nengo, we do this in a try/except.
@tbekolay
Copy link
Member Author

We reverted the model privatization in nengo/nengo#1210, so I took out the commit here making those changes. What's left is pretty small, but still needed for Nengo master at the moment! Once this is merged I'll do the final checks and go ahead with releases.

@hunse hunse merged commit 02c7d5c into master Nov 29, 2016
@hunse hunse deleted the update-230 branch November 29, 2016 20:25
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.

3 participants