scidash / neuronunit

A package for data-driven validation of neuron and ion channel models using SciUnit
http://neuronunit.scidash.org
38 stars 24 forks source link

ChannelModel class returns ReceivesSquareCurrent as failed capability for EGL-19.channel model #221

Open NeZanyat opened 5 years ago

NeZanyat commented 5 years ago

Url to file https://raw.githubusercontent.com/openworm/ChannelWorm2/master/NML2_models/EGL-19.channel.nml

gidili commented 5 years ago

@rgerkin this makes it impossible for us to create an instance of ChannelModel with this file. Something you can fix?

rgerkin commented 5 years ago

@gidili

tl;dr I should just remove "ReceivesSquareCurrent" as a capability for this model class, because it isn't really even doing that! It is just actually doing voltage steps. And I'm not sure what those capability checks would even look like, see below:

I realize now that in order to run this I was using pyNeuroML's channel analyzer module, which takes this .nml file and wraps it with a LEMS template that provides the ability to inject current. I put the resulting LEMS file [here]https://github.com/scidash/neuronunit/blob/dev/neuronunit/examples/model_zoo/LEMS_Test_ca_boyle.xml) and you will see that it had the original .nml file as an include.

Naturally when you check for extra capabilities on the site from this .nml file alone, they will fail because the ability to inject current has not been added, i.e. there is no way in the .nml file on its own to inject current.

Ideally, the XML template in pyNeuroML would set up current injection via a pulseGenerator tag as in the Izhikevich example, in which case the capability check would then succeed (on the .xml file), but instead it does so by defining a "vClampedCell" which has its own LEMS-defined dynamics, and I don't think it is realistic to parse those dynamics to determine whether a cell has a certain capability. However, it might be sufficient to just check for a vClampedCell element as an alternative to a pulseGenerator tag, and hope that there is some convention, at least for pyNeuroML, of using such an element in this type of simulation experiment. This could be the basis of a "ReceivesSquareVoltage" capability in the future.

But for now I think I should just remove that capability because its implementation is really tied up in the model file in ways that I should coordinate with pyNeuroML people (i.e. Padraig) before trying to solve one specific case.

What do you think?

NeZanyat commented 5 years ago

A bit more things:

  1. backend parameter is missing here https://github.com/scidash/neuronunit/blob/dev/neuronunit/models/channel.py#L15 but it's required because in other case it will cause an error

  2. We still have to remove this capability https://github.com/scidash/neuronunit/blob/dev/neuronunit/models/lems.py#L26

rgerkin commented 5 years ago

@NeZanyat I've implemented (1) in 94f3202 but I don't understand why (2) needs to be removed for ChannelModel to work. The extra_capability_checks only apply to capabilities claimed by the model, i.e. it will only fail if the a capability which is a key in extra_capability_checks is also a parent class of the model. Since ChannelModel does not have ReceivesSquareCurrent as a capability, then it shouldn't fail this check. Am I mistaken, or are you talking about a different model?

gidili commented 5 years ago

Hi @rgerkin ChannelModel itself doesn't have ReceivesSquareCurrent as a capability but inherits from LEMSModel that does have it as an extra capability check, so we are still finding that capability showing up as a failed extra capability with the same model when we call this method. Are we missing something?

rgerkin commented 5 years ago

@NeZanyat @gidili OK, I see. failed_extra_capabilities was too strict. extra_capability_checks may have many Capability:function mappings, but these should only be checked against Capabilities actually claimed by the model. LEMSModel.extra_capability_checks provides such a mapping for ReceivesSquareCurrent, but if a model does not claim the ReceivesSquareCurrent capability than this check should be ignored.

In the notebook or command line, I never call failed_extra_capabilities because I don't have a need to, which is why the ChannelModel example worked for me. But since you need it for the website, I have made the change in scidash/sciunit@06d84c5, which checks to see if the model actually claims the capability (i.e. is a subclass of it) before reporting that it failed. Let me know if this fixes the problem.

gidili commented 5 years ago

Thanks @rgerkin that'll definitely do it. We will give this a shot asap and report back! 🙏