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

New name system #52

Merged
merged 5 commits into from
Jun 13, 2013
Merged

New name system #52

merged 5 commits into from
Jun 13, 2013

Conversation

mstimberg
Copy link
Member

To get this out of the way, I implemented the changes to the naming system we discussed in #47 . Classes no longer have a basename attribute, instead they simply define e.g. neurongroup* as the default value for the name keyword argument. Internal objects that don't take a name argument might construct a name themselves, e.g. the Tresholder object calls the BrianObject initializer with name=group.name+'_'+'thresholder*'.

Marcel Stimberg added 2 commits June 12, 2013 16:59
…"basename" system and instead have default arguments like "neurongroup*". Does no longer automatically use source/target attributes to come up with a name. Closes #47.
@ghost ghost assigned thesamovar Jun 12, 2013
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling de0572d on name_system into 09d6478 on master.

@thesamovar
Copy link
Member

Thanks for writing this Marcel. One slight issue before I merge. For GroupCodeRunner you can pass a name argument, but this doesn't represent the actual name it will have because it prepends the group name. We should either (1) document this in the name argument (which currently doesn't mention the fact that the actual name will be a compound name), or (2) change the behaviour so that the default behaviour is name=None and then it constructs a compound name, or you specify the full name. I marginally prefer (2) I think because then the name argument behaves consistently everywhere it is used.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1de9c14 on name_system into 09d6478 on master.

@mstimberg
Copy link
Member Author

While writing it I first thought of doing (2), but then it wouldn't be possible to have a more specific name automatically, i.e. not neurongroup_coderunner but neurongroup_stateupdater, neurongroup_resetter, etc. For this we'd have to do the name construction in every subclass and this would be a bit repetitive. It wouldn't be too bad on the other hand, so maybe it's better to do it like this for consistency. My reasoning why I thought I could "get away with it" was that GroupCodeRunner is never called by the user. In the only place where it indirectly is, in the runner method, I actually implemented the variant (2). Which makes me wonder now whether the created name will have the group name twice...? I'll change it to make it consistent.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 4eb02ff on name_system into 09d6478 on master.

@thesamovar
Copy link
Member

Nice! Merging now.

thesamovar added a commit that referenced this pull request Jun 13, 2013
@thesamovar thesamovar merged commit bfb59d5 into master Jun 13, 2013
@thesamovar thesamovar deleted the name_system branch June 13, 2013 02:46
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.

3 participants