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.

Sign up for GitHub

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

Jump to bottom

Python nrn.Section.cell() changed from no to weak reference. #905

Merged
merged 4 commits into from
Jan 10, 2021
Merged

Python nrn.Section.cell() changed from no to weak reference. #905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
532b08f
Python nrn.Section.cell() changed from no to weak reference.
nrnhines Jan 9, 2021
c7c797c
Merge branch 'master' into section-cell-weakref
nrnhines Jan 9, 2021
cba7943
Merge branch 'master' into section-cell-weakref
pramodk Jan 10, 2021
1a47a56
Merge branch 'master' into section-cell-weakref
pramodk Jan 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions share/lib/python/neuron/tests/test_neuron.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,25 @@ def testSectionArgOrder(self):
soma = h.Section('soma')
assert soma.name() == 'soma'

def testSectionCell(self):
""" Section.cell() internally referenced as weakref."""
err = -1
try:
soma = h.Section(cell="foo", name="soma")
err = 1
except:
err = 0
assert err == 0
class Cell():
def __str__(self):
return "hello"
c = Cell()
soma = h.Section(cell=c, name="soma")
assert soma.name() == "hello.soma"
assert soma.cell() == c
del c
assert soma.cell() == None

Copy link
Member

Choose a reason for hiding this comment

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

Haven't tested, but from the code it looks like soma.name() is still "hello.soma" after the cell is deleted. Is that the "right" behavior?

I'm leaning towards it's the right thing, but I wanted to raise the question.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was perplexed by this case also. It represents an earlier misuse of the cell=... argument in that the section is supposed to be referenced by the container and destroyed when the container is destroyed. I suppose it would be possible to invalidate the section in the case where the section lives outside the cell but I don't think that is the correct behavior either.
The alternative of removing that part of the name so that it suddenly appears to be a section at the "top level" I think is inappropriate. Augmenting the cell part of the name so that it indicates the cell has been destroyed I don't think is worthwhile. Another possibility is to generate a warning or an error if this "misuse" happens. As the name of a Python Section is not entirely substantive (merely a label for gui display) I tend toward leaving it as is.

def testSectionListIterator(self):
"""As of v8.0, iteration over a SectionList does not change the cas"""
# See issue 509. SectionList iterator bug requires change to
Expand Down
75 changes: 44 additions & 31 deletions src/nrnpython/nrnpy_nrn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ extern int has_membrane(char*, Section*);
typedef struct {
PyObject_HEAD Section* sec_;
char* name_;
PyObject* cell_;
PyObject* cell_weakref_;
} NPySecObj;
NPySecObj* newpysechelp(Section* sec);

Expand Down Expand Up @@ -137,17 +137,6 @@ static char* pysec_name(Section* sec) {
if (sec->prop) {
NPySecObj* ps = (NPySecObj*)sec->prop->dparam[PROP_PY_INDEX]._pvoid;
buf[0] = '\0';
#if 0 // full name in ps->name because of nrnpy_pysecname2sec_remove
if (ps->cell_) {
PyLockGIL lock;

PyObject* cell = PyObject_Str(ps->cell_);
Py2NRNString str(cell);
Py_DECREF(cell);
char* cp = str.c_str();
sprintf(buf, "%s.", cp);
}
#endif
char* cp = buf + strlen(buf);
if (ps->name_) {
sprintf(cp, "%s", ps->name_);
Expand All @@ -162,10 +151,16 @@ static char* pysec_name(Section* sec) {

static Object* pysec_cell(Section* sec) {
if (sec->prop && sec->prop->dparam[PROP_PY_INDEX]._pvoid) {
PyObject* cell =
((NPySecObj*)sec->prop->dparam[PROP_PY_INDEX]._pvoid)->cell_;
if (cell) {
return nrnpy_po2ho(cell);
PyObject* cell_weakref =
((NPySecObj*)sec->prop->dparam[PROP_PY_INDEX]._pvoid)->cell_weakref_;
if (cell_weakref) {
PyObject* cell = PyWeakref_GetObject(cell_weakref);
if (!cell) {
PyErr_Print();
hoc_execerror("Error getting cell for", secname(sec));
}else if (cell != Py_None) {
return nrnpy_po2ho(cell);
}
}
}
return 0;
Expand All @@ -187,8 +182,19 @@ static int NPySecObj_contains(PyObject* sec, PyObject* obj) {
}

static int pysec_cell_equals(Section* sec, Object* obj) {
PyObject* po = ((NPySecObj*)sec->prop->dparam[PROP_PY_INDEX]._pvoid)->cell_;
return nrnpy_ho_eq_po(obj, po);
if (sec->prop && sec->prop->dparam[PROP_PY_INDEX]._pvoid) {
PyObject* cell_weakref = ((NPySecObj*)sec->prop->dparam[PROP_PY_INDEX]._pvoid)->cell_weakref_;
if (cell_weakref) {
PyObject* cell = PyWeakref_GetObject(cell_weakref);
if (!cell) {
PyErr_Print();
hoc_execerror("Error getting cell for", secname(sec));
}
return nrnpy_ho_eq_po(obj, cell);
}
return nrnpy_ho_eq_po(obj, Py_None);
}
return 0;
}

static void NPySecObj_dealloc(NPySecObj* self) {
Expand All @@ -198,6 +204,7 @@ static void NPySecObj_dealloc(NPySecObj* self) {
nrnpy_pysecname2sec_remove(self->sec_);
delete[] self->name_;
}
Py_XDECREF(self->cell_weakref_);
if (self->sec_->prop) {
self->sec_->prop->dparam[PROP_PY_INDEX]._pvoid = 0;
}
Expand Down Expand Up @@ -270,24 +277,32 @@ static int NPySecObj_init(NPySecObj* self, PyObject* args, PyObject* kwds) {
delete[] self->name_;
}
self->name_ = 0;
self->cell_ = 0;
self->cell_weakref_ = 0;
char* name = 0;
PyObject* cell = 0;
// avoid "warning: deprecated conversion from string constant to char*"
// someday eliminate the (char**) when python changes their prototype
if (!PyArg_ParseTupleAndKeywords(args, kwds, "|sO", (char**)kwlist,
&name, &self->cell_)) {
&name, &cell)) {
return -1;
}
// note that we are NOT referencing the cell
if (cell && cell != Py_None) {
self->cell_weakref_ = PyWeakref_NewRef(cell, NULL);
if (!self->cell_weakref_) {
return -1;
}
}else{
cell = 0;
}
if (name) {
size_t n = strlen(name) + 1; // include terminator
#if 1
if (self->cell_) {

if (cell) {
// include cellname in name so nrnpy_pysecname2sec_remove can determine

PyObject* cell = PyObject_Str(self->cell_);
cell = PyObject_Str(cell);
if (cell == NULL) {
Py_XDECREF(self->cell_weakref_);
return -1;
}
Py2NRNString str(cell);
Expand All @@ -296,9 +311,7 @@ static int NPySecObj_init(NPySecObj* self, PyObject* args, PyObject* kwds) {
n += strlen(cp) + 1; // include dot
self->name_ = new char[n];
sprintf(self->name_, "%s.%s", cp, name);
}else
#endif
{
}else{
self->name_ = new char[n];
strcpy(self->name_, name);
}
Expand Down Expand Up @@ -841,7 +854,7 @@ NPySecObj* newpysechelp(Section* sec) {
pysec->sec_ = sec;
section_ref(sec);
pysec->name_ = 0;
pysec->cell_ = 0;
pysec->cell_weakref_ = 0;
}
return pysec;
}
Expand Down Expand Up @@ -959,8 +972,8 @@ static PyObject* pysec_wholetree(NPySecObj* const self) {

static PyObject* pysec2cell(NPySecObj* self) {
PyObject* result;
if (self->cell_) {
result = self->cell_;
if (self->cell_weakref_) {
result = PyWeakref_GET_OBJECT(self->cell_weakref_);
Py_INCREF(result);
} else if (self->sec_->prop && self->sec_->prop->dparam[6].obj) {
result = nrnpy_ho2po(self->sec_->prop->dparam[6].obj);
Expand Down Expand Up @@ -1461,7 +1474,7 @@ static Object* seg_from_sec_x(Section* sec, double x) {
pysec = (NPySecObj*)psection_type->tp_alloc(psection_type, 0);
pysec->sec_ = sec;
pysec->name_ = 0;
pysec->cell_ = 0;
pysec->cell_weakref_ = 0;
Py_INCREF(pysec);
pseg->pysec_ = pysec;
}
Expand Down