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

Allow python section.disconnect(). #1451

Merged
merged 2 commits into from
Sep 8, 2021
Merged

Allow python section.disconnect(). #1451

merged 2 commits into from
Sep 8, 2021

Conversation

nrnhines
Copy link
Member

@nrnhines nrnhines commented Sep 7, 2021

Raise error if arg given to the hoc functions disconnect or
delete_section.

There are many such functions. Should this PR raise errors for those as well?
@ramcdougal , I thought you did a bunch of these but I can't find the changeset.

Closes #1447

A test of this has been added to test_basic.py and prints:

CHECKING: h.disconnect(sl[2])
NEURON: disconnect takes no positional arguments and disconnects the HOC currently accessed section. If using Python, did you mean a named arg of the form sec=section? Or you can use section.disconnect().
...
CHECKING: h.delete_section(sl[2])
NEURON: delete_section takes no positional arguments and deletes the HOC currently accessed section. If using Python, did you mean a named arg of the form, sec=section?

I think most of the offending functions could use one or the other of those styles, so they could become function calls that check if an arg exists and raise an error with the name substituted in the two or three places.

Raise error if arg given to the hoc functions disconnect or
delete_section.
@ramcdougal
Copy link
Member

I'd argue that any systematic change to check for the wrong number of arguments should be in a separate pull request. The main feature here is a new section method...

I think in the past I've made one off changes to raise an error on a specific function; nothing systematic. You could reasonably do that here if this pull request was about improving section disconnect.

Copy link
Member

@ramcdougal ramcdougal 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.

Stylistically, I wouldn't have created a variable x in the test because I dislike single-variable names because it gives me no intuition about why x == i but that's just me.

Copy link
Contributor

@Helveg Helveg left a comment

Choose a reason for hiding this comment

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

Current set of changes looks good, clear error message.

Some small comments in the tests about what is being tested might be useful. Like "Check that disconnected section has 2-piece subtree"

@nrnhines nrnhines merged commit 7eaece1 into master Sep 8, 2021
@nrnhines nrnhines deleted the hines/disconnect branch September 8, 2021 17:19
@alexsavulescu alexsavulescu mentioned this pull request Mar 22, 2022
15 tasks
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.

Section disconnect function
3 participants