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

Implemented new spikespace #97

Merged
merged 1 commit into from
Aug 19, 2013
Merged

Implemented new spikespace #97

merged 1 commit into from
Aug 19, 2013

Conversation

thesamovar
Copy link
Member

No description provided.

@ghost ghost assigned mstimberg Aug 19, 2013
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 907675d on spikespace into 6ed4d2b on master.

@thesamovar
Copy link
Member Author

Now that I know the Travis build passed I'm just going to go ahead and merge this in the interests of saving time.

thesamovar added a commit that referenced this pull request Aug 19, 2013
@thesamovar thesamovar merged commit 38f24b8 into master Aug 19, 2013
@thesamovar thesamovar deleted the spikespace branch August 19, 2013 03:54
@mstimberg
Copy link
Member

Looks good to me and nicely simplifies the C++ code. I changed one small thing: _spikespace shouldn't be a non-constant AttributeVariable, it can be a simple ArrayVariable instead. One of the advantages of the _spikespace technique is that the reference does not change, therefore there is no need to execute getattr(group, '_spikespace') at every timestep and insert it into the namespace.

I guess we should get rid of the spikes attribute / Variable completely to make sure it is not used anywhere?

@thesamovar
Copy link
Member Author

Nice! Yes, I left spikes because I didn't want to break any code but I guess it should go. Or maybe we want to keep it as it is, a property so that users can use it without needing to know about the spikespace implementation?

mstimberg pushed a commit that referenced this pull request Aug 25, 2013
No more _spikes attribute (replaced by _spikespace which was introduced with #97)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants