NeuroML / pyNeuroML

A single package in Python unifying scripts and modules for reading, writing, simulating and analysing NeuroML2/LEMS models.
https://docs.neuroml.org/Userdocs/Software/pyNeuroML.html
GNU Lesser General Public License v3.0
36 stars 30 forks source link

[Bug] pynml-modchananalysis: `ZeroDivisionError: float division by zero` #202

Closed sanjayankur31 closed 11 months ago

sanjayankur31 commented 1 year ago

Describe the bug Running pynml-modchananalysis results in a ZeroDivisionError: float division by zero error.

To Reproduce Run:

pynml-modchananalysis SK -stepV 5 -temperature 6.3 -v

on this mod file:

https://github.com/sanjayankur31/Human-L2-3-Cortical-Microcircuit/blob/feat/neuroml/NeuroML2/mod/SK.mod

Expected behavior Should analyse mod file and produce graphs

Observed behavior Crashes with error.

System information:

Additional context Full command output:

log.txt

borismarin commented 1 year ago

I was having a go at this issue (which is probably due to mishandled Ca dependence), but after running pynml-modchananalysis SK -stepV 5 -temperature 6.3 -v I get a different error:

Going to test channel: SK
Starting NEURON in Python mode...
No Ca mechanism present...
States found in mod file: ['z']
Traceback (most recent call last):
  File "/home/boris/.venv/pynml/bin/pynml-modchananalysis", line 33, in <module>
    sys.exit(load_entry_point('pyNeuroML', 'console_scripts', 'pynml-modchananalysis')())
  File "/home/boris/repos/pyNeuroML/pyneuroml/neuron/analysis/HHanalyse.py", line 201, in main
    sec.insert(str(chanToTest))
ValueError: argument not a density mechanism name.

I get this same message for both master and development. For which branch was the bug originally reported? Is mod parsing logic being changed?

borismarin commented 1 year ago

Nevermind the last comment, @sanjayankur31 - it is just an instance of #165. I'll keep going (and potentially fix 165 as well in the process...)

borismarin commented 1 year ago

I've created a PR (#216) which fixes this as well as #165. Important: the kinetics for that particular channel (SK) depends only on [Ca]. A pure voltage clamp sweep (which is what channelanalysis does) in a cell without additional Ca dynamics will lead to useless curves. We should either print a warning (detecting USEION ca READ cai?) or create a better analyser that can do something like "Ca clamps". Or maybe both?

sanjayankur31 commented 1 year ago

Yeh, in the short run, I think a warning with a sensible default value of [Ca] maybe be a conservative way to go? Could also just error out and say "use .... to set calcium conc" if we want to make the user provide a [Ca]?

borismarin commented 1 year ago

Even if the user sets a [Ca] value, for channels like this one (whose dynamics only depend on Ca, not in V) he will get flat curves that look "wrong" - even though they are technically correct. So I guess a warning is indeed important. It could simply pop up for every Ca dependent channel, which can easily be detected via USEION.

OTOH, that's very conservative. A less agressive solution would involve determining if [Ca] is the only variable in the derivative, but that requires symbolic expression parsing (I had written a nmodl parser in python that can do things like that), which will not be cleanly doable with regexes. But that might be too much for such a "niche" usage of pynml - unless the whole Ca clamping logic is also shared with pure neuroml channels.

Em seg, 13 de mar de 2023 08:42, Ankur Sinha @.***> escreveu:

Yeh, in the short run, I think a warning with a sensible default value of [Ca] maybe be a conservative way to go? Could also just error out and say "use .... to set calcium conc" if we want to make the user provide a [Ca]?

— Reply to this email directly, view it on GitHub https://github.com/NeuroML/pyNeuroML/issues/202#issuecomment-1465989594, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA2K632SW43HSS5G5IDDQNTW34B2LANCNFSM6AAAAAAR2M3RV4 . You are receiving this because you commented.Message ID: @.***>