-
Notifications
You must be signed in to change notification settings - Fork 52
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
[MRG] check section lengths match L-parameter #373
Conversation
Codecov Report
@@ Coverage Diff @@
## master #373 +/- ##
==========================================
- Coverage 89.93% 89.81% -0.12%
==========================================
Files 16 15 -1
Lines 2930 2897 -33
==========================================
- Hits 2635 2602 -33
Misses 295 295
Continue to review full report at Codecov.
|
for pt in p_secs[sec_name]['sec_pts']: | ||
h.pt3dadd(pt[0] + dx, | ||
pt[1] + dy, | ||
pt[2] + dz, 1, sec=sec) | ||
# with pt3dconst==0, these will alter the 3d points defined above! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I suppose this essentially functions to define the direction in which the section points?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That must be it, yes, though the documentation manages to be pretty unclear...
hnn_core/tests/test_cells_default.py
Outdated
@@ -1,6 +1,7 @@ | |||
import pytest | |||
|
|||
from neuron import h | |||
from numpy import allclose, abs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from numpy import allclose, abs | |
import numpy as np |
more standard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other issue is that you are overriding the abs
that comes natively from python that may lead to unexpected issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, bad form...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix this and I'll merge :)
hnn_core/tests/test_cells_default.py
Outdated
# check that after building, the vertical sections have the length | ||
# specified in get_L5Pyr_params_default (or overriden in a params file). | ||
# Note that the lengths implied by _secs_L5Pyr are completely ignored: | ||
# NEURON extends the sections as needed to match the sec.L 's | ||
vertical_secs = ['basal_1', 'soma', 'apical_trunk', 'apical_1', 'apical_2', | ||
'apical_tuft'] | ||
for sec_name in vertical_secs: | ||
sec = l5p.sections[sec_name] | ||
vert_len = abs(sec.z3d(1) - sec.z3d(0)) | ||
assert allclose(vert_len, sec.L) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test passes on master
... in other words it's not testing the new functionality
this is why I suggest starting by writing the test first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or I suppose the point of this PR is only to clarify the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this is intended to allow us to close #330 knowing that it won't bite us in the rear end when we start accepting section points other than those hard-coded in the private functions _secs_*{Pyr|Basket}
@@ -325,10 +325,12 @@ def _create_sections(self, p_secs, topology): | |||
self.sections[sec_name] = sec | |||
|
|||
h.pt3dclear(sec=sec) | |||
h.pt3dconst(0, sec=sec) # be explicit, see documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed when there is h.define_shape
? I feel one is enough ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither are really needed (as you pointed out, the code worked before)
pt3dconst
is set to '0' by default; I thought I'd make it explicit here to remind ourselves.- from the doc of pt3dadd:
Note: When L is changed, h.define_shape() should be executed to adjust the 3-d info so that branches appear connected.
I think it hasn't been a problem because none of our dendrites branch. Or do the last too basal ones...? I'm uncertain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can I just put it on record that I hate strongly dislike Neuron documentation :) Instead of raising sane errors, the onus is on the user to make sure things are executed in certain order with statements like "this may happen if you do this, but it may not be so bad ...".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll counter-sign that one, and you can pick an even stronger word if you'd like ;)
Thanks @cjayb ! |
Closes #330 by adding a test to check
sec.L
matches the difference insec.z3d(1) - sec.z3d(0)
.I added a few lines to
Cell
based on the NEURON docs forpt3dadd
: