-
Notifications
You must be signed in to change notification settings - Fork 0
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
Nmodl 1247 #20
Nmodl 1247 #20
Conversation
static inline void* mem_alloc(size_t num, size_t size, size_t alignment = 16) { | ||
void* ptr; | ||
posix_memalign(&ptr, alignment, num*size); | ||
memset(ptr, 0, size); |
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.
We allocated num*size
bytes and are zeroing size
bytes. Omar might have a PR for this.
table/neuron/table.cpp
Outdated
auto* const _ml = &_lmr; | ||
auto* _thread = _ml_arg->_thread; | ||
for (int id = 0; id < nodecount; id++) { | ||
|
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.
We'll need to find what printing this empty line.
table/neuron/table.cpp
Outdated
} | ||
|
||
|
||
void check_sigmoid1_tbl(_nrn_mechanism_cache_range* _ml, tbl_Instance& inst, size_t id, Datum* _ppvar, Datum* _thread, NrnThread* _nt) { |
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 function performs a lazy update, not a check. Are we free to change the name?
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 think it can be whatever, but due to merging BlueBrain/nmodl#1251 the changes will also have a minor impact on coreNEURON codegen.
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.
Note that in NEURON there's the nrn_thread_table_check_t
object (link), so I just went with that convention as coreNEURON codegen uses the same naming.
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.
Okay, however, please create a ticket. It can include all the other naming issues pointed out on nmodl-references
. This is all extremely simple (linear interpolation) but not easy to read.
table/neuron/table.cpp
Outdated
make_table = false; | ||
inst.global->tmin_sigmoid1 = -100.0; | ||
double tmax = 100.0; | ||
double dx = (tmax-inst.global->tmin_sigmoid1) / 100.; |
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 one place where one can quite nicely see that the interpolation variable (a voltage) doesn't have a name. I've seen, t
, x
, \xi
and theta
. While also using t_
as a prefix signifying "tabulated".
inst.sig[id] = inst.global->t_sig[index]; | ||
return 0; | ||
} | ||
int i = int(xi); |
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 size_t
the preferred integer type for this use?
return 0; | ||
} | ||
double xi = inst.global->mfac_sigmoid1 * (v - inst.global->tmin_sigmoid1); | ||
if (isnan(xi)) { |
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 is an odd check, because we can know if mfac
or tmin
are nan
much earlier (during codegen) and I can't remember us asserting that v
is valid in any other context.
|
||
|
||
inline int sigmoid1_tbl(_nrn_mechanism_cache_range* _ml, tbl_Instance& inst, size_t id, Datum* _ppvar, Datum* _thread, NrnThread* _nt, double v){ | ||
if (inst.global->usetable == 0) { |
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 puts a lot of faith in the optimizer. If it can't see that inst.global->usetable
is likely a constant, it can't even attempt to lift this branch outside of a loop. Which I suspect will cause it to fail to vectorize.
Many of the comments can be addressed by creating issues in BlueBrain/nmodl. |
No description provided.