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
Cu quantum mps #17
Cu quantum mps #17
Changes from 27 commits
9eb93cf
76f61bc
3cb0fec
a17d8e6
b043e6a
4da70db
ac712d2
6a22db0
73d65a6
fc3e0c2
ca6d579
ce018ab
32611cd
c15e4ca
32bee13
139748d
696f052
7abb82c
a42ba15
c3809d3
cb21d1d
3fafe2b
89bdbfb
78246cf
1dba4c3
d81cce5
d558609
ba442c9
4902195
b84ccad
e91045f
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.
Sooner or later we'll start using Pydocstring, and it will check that every first line will end with a full stop.
(this is just an irrelevant remark, because this PR is already pretty good, so we can improve even some details).
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 believe CuPy arrays to be immutable, so it should not be a problem.
But try to confirm it, because, otherwise, you'd be generating many references to the same object, that might have counter-intuitive effects (that is even if you modify just one, all the others will change as well.
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.
It's not necessarily bad, but it is just simpler to avoid the assignment, since you're immediately returning (unless you're very motivated to give it a name, but the name will usually be very close to the function one)
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.
Tell the user that the operation will be performed in-place.
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.
Since the operation is in-place, you do not need to return the modified object, since it's the one passed by the user.
(and there should be only one way of doing something, so it would be confusing to have the two references around)
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 do you need all these assignments to
self
?If possible, keep everything local (in this case local to the
__init__
method).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.
Just a proposal to simplify the method names: since the class is already named
MPSContractionHelper
, you could avoid prefixing methods withcontract_
(since it is reasonable that anMPSContractionHelper.norm
method will make a contraction to give you the norm).However, it is just a proposal and definitely subject to opinions. So, feel free to keep as it is.
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.
To slightly improve the name's expresiveness, at very little expense ^^
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.
Same of above