Skip to content
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

Move nsubsystems from QOBase #19

Merged
merged 5 commits into from
Sep 21, 2023
Merged

Move nsubsystems from QOBase #19

merged 5 commits into from
Sep 21, 2023

Conversation

ba2tro
Copy link
Contributor

@ba2tro ba2tro commented Sep 19, 2023

No description provided.

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #19 (e98af67) into main (f535bbb) will decrease coverage by 0.18%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main      #19      +/-   ##
==========================================
- Coverage   17.55%   17.38%   -0.18%     
==========================================
  Files          12       12              
  Lines         393      397       +4     
==========================================
  Hits           69       69              
- Misses        324      328       +4     
Files Changed Coverage Δ
src/embed_permute.jl 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Krastanov
Copy link
Collaborator

not all AbstractKet have a basis slot in their structure. Here we should have only the function declaration, but these implementations should either make no assumption about the struct or should be in QuantumOpticsBase.

Let's say we keep them here, in which case instead of s.basis we should have basis(s).

@Krastanov
Copy link
Collaborator

Basis and CompositeBasis are defined here, so they are all right.

Also bump the patch version number so we can release it promptly.

@Krastanov
Copy link
Collaborator

could you bump the patch version and add to the changelog?


nsubsystems(s::AbstractKet) = nsubsystems(basis(s))
function nsubsystems(s::AbstractOperator)
s.basis_l == s.basis_r || throw(ArgumentError("`nsubsystem(::AbstractOperator)` is well defined only if the left and right bases are the same"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as with ::AbstractKet. We can not rely on AbstractOperator to have .basis_l and .basis_r fields.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a basis(op) call will raise an error if the left and right basis are not the same, so maybe just use it directly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we stick to throwing the current error message with the basis(op) in a try catch or just let the basis(op) error show when the bases are unequal?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to stick to the current message, it is fine to rely on whatever message basis throws. Just check that it actually works (you know, never trust me, I can always be wrong ;)

@Krastanov Krastanov merged commit 35e721b into qojulia:main Sep 21, 2023
8 of 11 checks passed
@ba2tro ba2tro deleted the nsub branch October 2, 2023 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants