search

AllenNeuralDynamics / aind-behavior-curriculum

Starter repository for behavior base primitives.
https://aind-behavior-curriculum.readthedocs.io
MIT License
1 stars 0 forks source link

Empty classes could be creating unnecessary layers #29

Open jeromelecoq opened 1 month ago

jeromelecoq commented 1 month ago

I am going through the repo with an eye for simplicity. why do we need this empty class?

See here : https://github.com/AllenNeuralDynamics/aind-behavior-curriculum/blob/bb28161608b4544e5f71e23c2190a6682f17dcaf/src/aind_behavior_curriculum/task.py#L19C22-L19C44

jeromelecoq commented 1 month ago

Another empty layer : https://github.com/AllenNeuralDynamics/aind-behavior-curriculum/blob/bb28161608b4544e5f71e23c2190a6682f17dcaf/src/aind_behavior_curriculum/curriculum.py#L30

bruno-f-cruz commented 1 month ago

Both of these classes should be seen as abstract classes (we can even inherit from the ABC module, but this would preclude users from using #30, which might not be a bad idea...).

Subclassing and building your own models is what affords validation (the argument is the same if you want to use aind-data-schemas, one could also just use a very large dictionary). If you don't subclass you can't impose a structure, if you cant impose a struct everything will be validated, best case scenario, to a dictionary. How do you propose and API validating Metrics that users send to an endpoint if we don't require a known structure? How to you ensure inter-operability across users and tools? We just run a check for known dictionary keys?

I understand, and agree, with the need for things to be "easy" and "user-friendly", but I feel this argument must be weighted against rigor and scalability. That being said, if there is a way to strike a better balance we should go for it, but I would like to see alternatives before removing this pattern.

jeromelecoq commented 1 month ago

It looks like this approach expect any user to understand all of those concepts and layers. I think I have already some experience with declaring behavior tasks with another package and I am confused. So any person that is new to tracking behavior will have to learn this new logic. As you said we have to balance rigor with usage. I am providing feedback on usage from a fresh angle. I am willing to put the effort to get to your level. But already I can say this is not easy.