-
Notifications
You must be signed in to change notification settings - Fork 6
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 test_bases from QuantumOpticsBase and reorganize a bit #37
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
===========================================
+ Coverage 17.08% 34.25% +17.16%
===========================================
Files 12 12
Lines 398 400 +2
===========================================
+ Hits 68 137 +69
+ Misses 330 263 -67 ☔ View full report in Codecov by Sentry. |
ab297e7
to
791017c
Compare
could you also bump the JET ignore count to 3. It seems it is the case on master 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.
Sorry for the delay in getting this reviewed!
This type of reordering is usually to be avoided for two reasons:
- It took me a lot of time to review the changes as moving this around leads to a ton of lines marked as modified (git diff is not very good about just showing moves).
- Using git blame in the future will be much more difficult.
With this in mind, what are the advantages of the reordering that make it worth to pay the price?
""" | ||
Abstract base class for all specialized bases. | ||
|
||
The Basis class is meant to specify a basis of the Hilbert space of the |
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.
The Basis class is meant to specify the Hilbert space of the
To avoid the confusion about basis vs space
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.
Hm it seems like even if Basis
will sometimes be abused to only refer to the Hilbert space of the given dimensionality, this shouldn't necessarily be reflected in the documentation.
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.
will sometimes be abused to only refer to the Hilbert space
I think there will be disagreement over "Basis
is abused to ..." vs "Basis
is not the best name to begin with because it is frequently meant to be used as ...". I do not have strong preferences on the exact phrasing, but documenting the fact that in this library Basis
might refer to a Hilbert space, not specifically to a Basis of a Hilbert space seems reasonable.
Maybe we can phrase it as "The Basis class is meant to specify a basis of the Hilbert space (and in abuse of notation occasionally it refers to the Hilbert space as a whole)"?
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.
occasionally -> frequently
This reorganization was helpful for me to get a better overview of exactly what kind of interface is being specified. Hence things like moving function definitions to |
That reasoning is fair. Something that will speed up review in the future would be to do a lot of small commits for each change -- that way for the reviewer and for various automatic tools it is much easier to detect typos (or even malicious changes, although that is not something we really need to worry about in our domain). |
I am in favor of merging, after you share your opinion on the basis documentation question above. No need to address it here if you find that inappropriate -- there will be plenty of opportunities to address it in the rest of the batch of PRs you have prepared to fix the variety of issues with Basis. Just let me know your preference. |
In preparation of more significant changes to the basis interface and abstract types being discussed across #12 #25 #26 #27 #33 #34 #35 #36 this PR adds the
test_bases
fromQuantumOpticsBase
, moves all theshow
methods to their own file, and does a little more reorganization. No logic should be affected with this change.