Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add QuantumKernelTrainer and KernelLoss classes #244
Add QuantumKernelTrainer and KernelLoss classes #244
Changes from 61 commits
b6399c7
a4803b2
7344b90
7a990cb
f86f844
4b58eb2
33b0d39
a7ae58f
2b524f2
2bee96e
72b4905
f6202c8
bad47a9
5949970
6833223
47f0d3e
1126bbd
859ad4f
d2323d8
45e2aa4
d40345e
6a3e042
2973ca4
67dbc22
afa7232
eb95125
7640bfb
f0506dc
7989965
1e468ee
c7b191e
fe34bfb
47267f1
30d4ec8
6921254
7e51267
bd417d1
33972a9
c946418
83b2330
5af811e
b57d1a4
84c38f3
676816b
a4e4203
2ec50d5
30395f2
c9ec0e3
eb90863
6214143
1f49459
66f04b2
52c2e61
3849e60
a24a563
199e3be
7131b7c
08de1b8
44e0c0e
67c4125
309dd52
0874a93
dd49cd3
28589ae
09c75bb
4f18589
000f7b4
fc99b56
d6bd020
d5c8bff
4ed685a
5505b61
a6625d4
c7d03c1
93f8cbb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can you please update loss type hints to be the same across the code? I guess it should be
Union[str, KernelLoss]
now.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.
Can be simplified:
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.
Why don't you want to instantiate a loss object right here? It would simplify the rest of the code, no need for additional checks whether it is a string or a callable.
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.
if
andelse
, if does nothing.ValueError
in the docstringThere 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.
I'm not sure I follow your if/else comment. This logic looks fine to me, what am I missing?
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.
I mean
if
clause has justpass
, whileelse
throws an error. Maybe better to swap them and makeif
to throw an error and then there's no need forelse
.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.
Oh yes, I set it up in this funky way to lay the way for future loss classes. Since there is only one right now, the logic is a bit strange.
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.
In the future, with more than one value you can do something like an if not self._loss in ["a", "b", "c"] etc so as to throw an exception if its not a recognised one, assuming its all done the same way. So that reversal and throwing an error if its not in the recognized set is still a simple one line thing and does not require a complicated if/else structure. Assuming this is all this if test is expected to do based on type.
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.
If the comment removed, does the code pass pylint/mypy checks?
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.
I'd suggest to rename to plain
fit
, make it morescikit-learn
compatible. See here: https://scikit-learn.org/stable/developers/develop.html, QKT is an estimator, I think, inscikit-learn
terms.