-
Notifications
You must be signed in to change notification settings - Fork 28
Add Cyclic Group Representation (CGR) class #182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Add a new base class to support future implementation of variations of MCR proposals.
There is a bug when args is a collection of collections instead of a plain tuple. In this case, the old args parsing was unable to search for block_size in nested structures containing BaseMCRTensor. For instance, let `a` and `b` be BaseMCRTensor variables. Calling `torch.stack((a, b))` results in error, as the `args` received in `__torch_function__()` is a nested tuple.
Add a new VSA class named Cyclic Group Representation (CGR). This class is similar to MCR, but differs in bundling.
Allow its usage in level and circular embeddings as done with MCRTensor.
Ensure both inputs are in the same shape.
The CGR should behave almost the same as MCR, but diverges in bundling. Implement a custom bundling test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening this PR, this would be a great addition to the library. Your implementation looks good, I just added some minor comments. Good catch of the __torch_function__
bug btw.
It would also be good to add a link to the documentation page of the Cyclic Group Representation to the README. Note that the link will not work now but will work as soon as this PR gets merged (assuming you add the documentation page). |
I can think of two approaches to making the documentation for each VSATensor in one page. One is to not have the def multibind(self) -> "MCRTensor":
"""Bind multiple hypervectors"""
return super().multibind() This will add the method and docstring to the documentation page. Then all you have to do to include the |
Shorter syntax and more readable.
No need to check block size in CGR/MCR functions as the __torch_function__() in BaseMCRTensor already checks it.
- Raise ValueError instead of RuntimeError
Include CGRTensor in the README and in the built docs.
I've just pushed some commits. Regarding your comments, Parameter checkingThe use of assertions and
CGR and MCR operations (bind, bundle, and similarity) must always be done with hypervectors adopting the same In fact, there might be a slight performance issue since Documentation
After some tests considering this option, I feel like repeating function signatures also isn't the way to go, as the code isn't DRY. Maybe let users check two doc pages and link |
Thank you for the edits.
When weighing between user experience or code style, I would suggest we lean towards a better user experience in favor of the coding best practices. Said plainly, the users don't care if our code is DRY but they get annoyed if they have to read the documentation of one class in two places. So I would suggest we implement all the methods that need documentation on the child classes and have them call |
Introduce a new VSA class. Solves #181.
Description
CGR is very similar to MCR, but differs in bundling. This pull request introduces:
Additionally, this commit also solves a bug in the
MCR: __torch_function__()
implementation. This function parsesargs
to search for MCRTensors and ensures they all have the sameblock_sizes
.torchhd/torchhd/tensors/mcr.py
Lines 420 to 424 in 6b7478d
However, it treats
args
as a shallow 1D container. In this case, the following snippet breaks the code:In this minimal example,
torch.stack()
calls__torch_function__
with a 2D tuple that can't be parsed.Checklist
The only problem at the moment is in the documentation. Due to the addition of
BaseMCRTensor
, the documentation of MCR and CGR is split, requiring users to read two mnual pages. Also, I wasn't able to add newCGRTensor
andBaseMCRTensor
classes into the proper table of contents.