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

WIP: Instance-less capabilities #205

Closed NeZanyat closed 5 years ago

rgerkin commented 5 years ago

@NeZanyat @gidili What is the purpose of this change?

It sounds like you don't want to instantiate the model class in order to check for the extra capabilities, but these extra capabilities are all things that can only be checked (e.g. checking to see if there is really a pulse_generator tag) at the level of the LEMS file, which means at the least you would need to know which LEMS file the user intends to use to instantiate the model, and you would need to be able to parse that LEMS file to see if those extra capabilities are implemented. This is most easily done from the instantiated model.

One exception I can see would be if you simply want to check to see what extra capabilities are going to be checked (for example, to create a warning or error message for users) rather than to actually check them. In that case the best implementation would be to just make extra_capability_checks a class attribute, rather than making a whole classmethod. It is a dictionary and therefore mutable and therefore shared across all instances of the class, but I think that's OK because it is just saying which checks should be run on instances of that class, not whether those checks passed.

NeZanyat commented 5 years ago

One exception I can see would be if you simply want to check to see what extra capabilities are going to be checked (for example, to create a warning or error message for users) rather than to actually check them.

That's the exact reason why I did this change.

In that case the best implementation would be to just make extra_capability_checks a class attribute, rather than making a whole classmethod.

You're right, but there is one thing which is preventing me to implement it in this way.

https://github.com/scidash/neuronunit/blob/dash/neuronunit/models/lems.py#L36 - at this line you setting up not the whole extra_capability_checks, but only one key. My guess is that you're doing this to avoid erasing of extra_capability_checks which was set by parent classes, so I had to implement it in way which save all checks from parents. That's why I did this as classmethod

rgerkin commented 5 years ago

@NeZanyat Is there any reason we cannot just make extra_capability_checks a class attribute rather than a classmethod? And also check for instance-level-specific checks:

class LEMSModel(...):
   extra_capability_checks = {cap.ReceivesSquareCurrent: 'has_pulse_generator'}

   def get_extra_capability_checks(self):
      checks = {}
      checks.update(self.extra_capability_checks)
      checks.update(getattr(self, '_extra_capability_checks', {})
      return checks
NeZanyat commented 5 years ago

@rgerkin of course we can, if there is no chance that it will break something. Parent extra checks or something like this.

NeZanyat commented 5 years ago

I'll do a bit different version for this and I will send new PR for this.