-
Notifications
You must be signed in to change notification settings - Fork 61
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
Creation of qibo.models.encodings
and implementation of unary_encoder
#1059
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
📢 Thoughts on this report? Let us know!. |
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, it is fine for me.
Just a question, I noticed the notation is slightly different from the paper you cite in the docstrings. Did you apply these modifications in order to correctly deploy the data loader?
It perfectly works btw, so it sounds good to me.
I don't know which part exactly you're referring to, but it's probably just personal preference. |
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.
Minor comments, looks good to merge as it is. The only thing that is slightly weird is that this is exposed as a model, but the name follows different convention than other models. Some users may expect from qibo.models import UnaryEncoder
. I get that you are following the function naming convention so it should be fine to keep as it is.
Checklist: