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.

Sign up for GitHub

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

Jump to bottom

Use CSR as default for expand_operator #227

Merged
merged 4 commits into from
Apr 3, 2024
Merged

Use CSR as default for expand_operator #227

merged 4 commits into from
Apr 3, 2024

Conversation

BoxiLi
Copy link
Member

@BoxiLi BoxiLi commented Dec 18, 2023

Transform the qobj to CSR only if it is Dense, in case more data types are introduced in the future.

@BoxiLi BoxiLi added this to the qutip-qip-0.3.1 milestone Dec 18, 2023
hodgestar
hodgestar approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Could we add a small test for this? E.g. check that a Dense operator really is expanded to a CSR? Otherwise looks good.

hodgestar
hodgestar reviewed Feb 11, 2024
View reviewed changes
Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test.

Why were the qiskit tests removed?

src/qutip_qip/operations/gates.py Show resolved Hide resolved
@BoxiLi
Copy link
Member Author

BoxiLi commented Feb 12, 2024

Deleting qiskit_test was a mistake

@BoxiLi
Copy link
Member Author

BoxiLi commented Feb 12, 2024 •
edited
Loading

And maybe I should make the same change to the function in qutip-v5 .
https://github.com/qutip/qutip/blob/fcb4117c25e22fe2f553a3bd6d0eda6458f6ff12/qutip/core/tensor.py#L416

@hodgestar
Copy link
Contributor

And maybe I should make the same change to the function in qutip-v5 . https://github.com/qutip/qutip/blob/fcb4117c25e22fe2f553a3bd6d0eda6458f6ff12/qutip/core/tensor.py#L416

I would say yes. Ideally the two functions should remain the same.

@BoxiLi BoxiLi force-pushed the expand_operator_csr branch from 15c4f1d to 3b9d6dd Compare April 2, 2024 01:36
@BoxiLi
Copy link
Member Author

BoxiLi commented Apr 2, 2024

@Ericgig Does the change look good to you? I'll make the same PR to the main qutip

@Ericgig
Copy link
Member

Ericgig commented Apr 2, 2024

I am fine with setting csr as the default type to expand_operator.

BoxiLi added 4 commits April 3, 2024 08:20
@BoxiLi
Use CSR as default for expand_operator
29388fd 
Transform the qobj to CSR only if it is Dense, incase more data types are introduced in the future.
@BoxiLi
Add optional dtype to expand_operator
fdb797a
@BoxiLi
Add tests
f60a06a
@BoxiLi
Align with the dtype setting in main qutip
ea6d7f7
@BoxiLi BoxiLi force-pushed the expand_operator_csr branch from 3b9d6dd to ea6d7f7 Compare April 3, 2024 06:21
@BoxiLi BoxiLi merged commit 045eaab into qutip:master Apr 3, 2024
11 of 12 checks passed
BoxiLi added a commit to BoxiLi/qutip-qip that referenced this pull request Apr 20, 2024
@BoxiLi
Merge pull request qutip#227 from BoxiLi/expand_operator_csr
9fa3d64 
Use CSR as default for expand_operator
@BoxiLi BoxiLi mentioned this pull request Apr 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hodgestar hodgestar hodgestar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
qutip-qip-0.3.1
Development

Successfully merging this pull request may close these issues.
3 participants
@BoxiLi @hodgestar @Ericgig