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

from neuron import coreneuron specifies CoreNEURON run properties #643

Merged
merged 7 commits into from
Jul 12, 2020

Conversation

nrnhines
Copy link
Member

coreneuron.nrncore_arg(tstop) returns str that can be used with
pc.nrncore_run(str) based on coreneuron properties and current
NEURON settings. User specifiable properties are enable, gpu,
cell_permute, warp_balance, verbose, and prcellstate. Args derived
from current NEURON settings are --mpi, --binqueue, --spkcompress,
--multisend, --ms_subinterval, and --ms_phases.

Closes #639

coreneuron.nrncore_arg(tstop) returns str that can be used with
pc.nrncore_run(str) based on coreneuron properties and current
NEURON settings. User specifiable properties are enable, gpu,
cell_permute, warp_balance, verbose, and prcellstate. Args derived
from current NEURON settings are --mpi, --binqueue, --spkcompress,
--multisend, --ms_subinterval, and --ms_phases.
@nrnhines nrnhines requested a review from pramodk July 11, 2020 13:40
@nrnhines nrnhines marked this pull request as draft July 11, 2020 13:41
Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

looks ok to me. but we can tune / update once psolve changes are in.

share/lib/python/neuron/coreneuron.py Show resolved Hide resolved
'''
Check type and value in range for the user properties.
'''
assert(type(tstop) is float)
Copy link
Member

Choose a reason for hiding this comment

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

if user set int then it is also treated as float?

Copy link
Member Author

@nrnhines nrnhines Jul 11, 2020

Choose a reason for hiding this comment

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

what do you think about

tstop = float(tstop) # error if can't be a float
enable = bool(enable)       
gpu = bool(gpu)
cell_permute = int(cell_permute)

I suppose the last two would never raise an error.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine. If we will need more elaborate option validation, we can extend it later.

The error handling definitely needs work if CoreNEURON does not work.
@pramodk
Copy link
Member

pramodk commented Jul 11, 2020

@nrnhines : is this ready for basic testing? If so, what are steps we need to do? e.g.

  • nrnivmodl & nrnivmodl-core
  • modify python script to import neuron and enable it

I wonder about cache_efficient(1). Should we set this if coreneuron is enabled?

@nrnhines
Copy link
Member Author

nrnhines commented Jul 11, 2020

It is ready for basic testing. Please examine the output of this test.

temp.py.txt

I wonder about cache_efficient(1)

It is tempting to check and set in neuron.coreneuron.nrncore_arg(tstop) but after setting it requires a stdinit(). That may be too far. (although it would almost be safe to set in enable if that was a function.)

@pramodk
Copy link
Member

pramodk commented Jul 11, 2020

It is tempting to check and set in neuron.coreneuron.nrncore_arg(tstop) but after setting it requires a stdinit().

Just thinking loud : stdinit / finitialize can check if coreneuron is enabled and then set it cache_efficient(1) as first thing?

@nrnhines
Copy link
Member Author

nrnhines commented Jul 11, 2020

I experimented and see that if I have changed cache_efficient(...) after stdinit and before a normal pc.psolve I get segmentation violation, but if it is between a stdinit and an enable=True pc.psolve then I get

NEURON: NEURON model internal structures for CoreNEURON are out of date. Make sure call to finitialize(...) is after cvode.cache_efficient(1))

So your idea would work unless they set enable after finitialize()

@nrnhines
Copy link
Member Author

nrnhines commented Jul 11, 2020

I might be able to fix both the normal psolve segmentation violation and the nrncore_run complaint of Model internal structures out of date by factoring out the first part of nrn_finitialize() to do the internal structure update (no data value changes but pointers get updated as well) One consequece would be that neuron.coreneuron.nrncore_arg(tstop) could safely call
h.cache_efficient(1)

@nrnhines
Copy link
Member Author

Another possibility to avoid the h.cache_efficient(1) issue is to make it unnecessary for transferring data to CoreNEURON. It really was mostly a convenience requirement to allow reasonably fast determination of what mechanism instance data a pointer is referencing. At any rate, for the purposes of this pull request, I think we can require that the user call h.cache_efficient(1) (if using CoreNEURON) prior to finitialize(). And finitialize() must be prior in all cases to the first call to pc.psolve(tstop). Or else suffer a meaningful error message to be implemented in the next changeset to this pull request.

If not, raises the error:
NEURON model internal structures are out of date
@pramodk
Copy link
Member

pramodk commented Jul 12, 2020

At any rate, for the purposes of this pull request, I think we can require that the user call h.cache_efficient(1) (if using CoreNEURON) prior to finitialize().

Agree. We can make this mandatory and can re-evaluate other possibilities later.

@nrnhines
Copy link
Member Author

Need any more for now? Looks like we are supporting

cvode.cache_efficient(1)
stdinit()
pc.psolve(tstop)

And deferring subsequent pc.psolve(tstop) to when we have data transfer back and forth between NEURON and CoreNEURON so that NEURON can change parameters between successive pc.psolve calls

@nrnhines nrnhines marked this pull request as ready for review July 12, 2020 17:52
Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

Some minor comments. Once these are answered/clarified, good to merge. (already approving)

share/lib/python/neuron/coreneuron.py Show resolved Hide resolved
share/lib/python/neuron/coreneuron.py Show resolved Hide resolved
src/nrniv/netpar.cpp Show resolved Hide resolved
src/nrniv/nrnbbcore_write.cpp Show resolved Hide resolved
src/nrniv/nrnbbcore_write.cpp Outdated Show resolved Hide resolved
src/nrnpython/nrnpy_hoc.cpp Outdated Show resolved Hide resolved
src/parallel/ocbbs.cpp Outdated Show resolved Hide resolved
src/parallel/ocbbs.cpp Show resolved Hide resolved
@nrnhines nrnhines merged commit 674dbac into master Jul 12, 2020
@nrnhines nrnhines deleted the coreneuron-module branch July 12, 2020 20:37
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.

Define NEURON to CoreNEURON API for HOC/Python execution
2 participants