Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

Initial support for matrix inverse in qdk_sim_experimental crate #920

Merged
merged 14 commits into from
Apr 8, 2022

Conversation

cgranade
Copy link
Contributor

@cgranade cgranade commented Jan 27, 2022

This PR begins work on #915 by adding a pure-Rust implementation of LU decompositions and matrix inverses, drawing from the MIT-licensed examples at https://github.com/cgranade/cursed-linalg. These linear algebra routines will be needed in order to implement expm without passing a LAPACK dependency through to end users.

The PR will appear very messy until #979 is merged, as it includes unrelated changes from main.

Note that this PR targets a feature branch, and further work on unit tests, integration tests, and perf benchmarks are expected before that feature is merged to main.

@cgranade cgranade requested a review from swernli January 27, 2022 19:47
@cgranade cgranade changed the title Initial support for expm in qdk_sim_experimental crate Initial support for matrix inverse in qdk_sim_experimental crate Apr 1, 2022
@cgranade
Copy link
Contributor Author

cgranade commented Apr 1, 2022

This PR has been updated to use a pure-Rust implementation of inv instead, please see updates to description above.

Copy link
Contributor

@kuzminrobin kuzminrobin left a comment

Choose a reason for hiding this comment

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

I'm learning Rust and still don't understand most of the code here. But at a glance I didn't see any thing to report.
Approving to unblock you, but I would recommend an approval from someone more knowledgeable than me.

@cgranade
Copy link
Contributor Author

cgranade commented Apr 4, 2022

@kuzminrobin: Thanks! If you're curious about any of the Rust-y parts, I'd be happy to sync with you and go over in more detail.

In the meantime, @swernli and @SamarSha, if you have any comments on Rust-specific points I'd greatly appreciate your feedback. Thanks!

Copy link
Contributor

@bamarsha bamarsha left a comment

Choose a reason for hiding this comment

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

I've only looked through half of the files so far.

Copy link
Collaborator

@swernli swernli left a comment

Choose a reason for hiding this comment

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

Some minor feedback below, but overall it looks great!

src/Simulation/qdk_sim_rs/Cargo.toml Outdated Show resolved Hide resolved
src/Simulation/qdk_sim_rs/src/tableau.rs Show resolved Hide resolved
src/Simulation/qdk_sim_rs/src/utils.rs Show resolved Hide resolved
@cgranade
Copy link
Contributor Author

cgranade commented Apr 8, 2022

@SamarSha: Wanted to double-check that all your feedback has been addressed before merging. Thanks!

@bamarsha
Copy link
Contributor

bamarsha commented Apr 8, 2022

Yes, looks good, thanks.

@cgranade
Copy link
Contributor Author

cgranade commented Apr 8, 2022

Awesome, thank you for all your detailed feedback! 💕

@cgranade cgranade merged commit 0b79432 into feature/opensim-continuous Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants