Ensure SP algebra matches vocab #240
Conversation
| @@ -63,6 +64,10 @@ def _get_algebra(cls, vocab, algebra): | |||
| algebra = vocab.algebra | |||
| elif vocab is not None and vocab.algebra is not algebra: | |||
| raise ValueError("vocab and algebra argument are mutually exclusive") | |||
| if not isinstance(algebra, AbstractAlgebra): | |||
There was a problem hiding this comment.
This seems unpythonic to me. Usually duck typing should be fine and enforcing that an algebra derives from AbstractAlgebra technically seems like a breaking change to me.
I would prefer to leave this check out (or just test for the protocol if that can be done easily). Of course, this enables a certain class of bugs, but this is a general problem of dynamic typing and there are probably many more instances in Nengo SPA where it would apply. I'd rather consider adding type annotations across the library (though this will probably require dropping support for Python 3.4 which has reached EOL a year ago).
There was a problem hiding this comment.
This was added because the unit test you created passed VtbAlgebra instead of VtbAlgebra() and things still worked (because the test didn't do anything with the pointers, like try to negate or add them).. moreover, when I did try to do something with them the error message was very cryptic.. so I figured if the mistake was that easy to make then something should be done to avoid having that happen again in the future.
There was a problem hiding this comment.
I guess we should change the AbstractAlgebra class to a proper ABC which should make the isinstance check succeed even if the algebra does not derive from AbstractAlgebra as long as all required methods are implemented.
There was a problem hiding this comment.
I made an additional commit to continue to support duck-typing for algebras, but deprecate it (so that in the future we can fully rely on the abstract base class).
when adding to a vocab. SemanticPointe.reinterpret can be used to add the pointer anyways. Mixing algebras is not supported without explicit casts. Fixes #239. Clarify that the created SPs keep their algebras with a test and some added comments. Also removed redundant check, and renamed variable names in check.
However, deprecate this feature to be able to remove the additional code for this later and make AbstractAlgebra a pure ABCMeta class. This allows to do type checking in the vocabulary without a breaking change. Due to the deprecation, I consider this worth a minor release and changed the changelog accordingly. squash! Continue to allow duck-typing for algebras Satisfy flake8
Motivation and context:
Mixing algebras is not supported without explicit casts. Thus raise an exception when this is tried.
SemanticPointe.reinterpret can be used to add the pointer anyways.
Fixes #239.
How has this been tested?
unit test
How long should this take to review?
Types of changes:
Checklist: