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
[on hold] Switch
convert_2q_block_matrix.rs
to usematmul
fromfaer
and an explicit version ofkron
#12193[on hold] Switch
convert_2q_block_matrix.rs
to usematmul
fromfaer
and an explicit version ofkron
#12193Changes from 9 commits
9c3ad66
52d2174
e920dfb
ae7336e
1e92755
3203678
a34927f
188a1c1
efb3876
36af13a
0f877a3
7754ebf
557f992
04ffe65
33d687b
df0edac
9a26ed9
065a002
b811808
2f85d24
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.
The
op_matrix.to_owned()
here is a bit weird to me. It feels like an unnecessary copy as we don't actually need an owned copy of the array from python here as it's only used in thematmul
call below. The previous logic was a bit ugly but was done that way to avoid copying withto_owned()
. The other option would be to maybe try something like:and change
result
to be aMatRef<c64>
instead of of aMat<c64>
. Although I'm not sure if the compiler would be happy with this (there might be a lifetime issue doing this. The other advantage is ifresult
becomes aMatRef
we can create the 4x4 identity matrix as a static and avoid the allocation (although in practice I'm not sure this ever gets used so it probably doesn't matter).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 have tried the code you suggested, and as you said, the compiler is not happy about it. The error message is "temporary value is freed at the end of this statement" referring to each case of the
match
where we useas_ref()
.Eric and I have been trying to fix it, but the only alternative we saw to avoid copying with
to_owned()
is to recover the previous logic, making theresult
matrix anOption<Mat<c64>>
and "duplicating" thematmul
call using the unwrapped value of theSome
orop_matrix
in case ofNone
. It's not as readable as now, but performance-wise should be better.Here is the
git diff
of the change. What do you think about doing something like this again? I suspect we will see this case intwo_qubit_decompose.rs
too.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.
FYI this is what we were trying to do:
Using
result.as_ref().unwrap_or(op_matrix)
does not work.result.as_ref
results in anOption<&Mat>
rather thanOption<MatRef>
and they are not the same thing.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.
Yeah, I think lets use this pattern, it's ugly but it lets us avoid a copy
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.
Should this not be
result.as_ref().map(|x| x.as_ref()).unwrap_op(op_matrix)
?Fwiw,
match result
is wrong because it's consuming theresult
, so there's nothing for the reference to refer to. The match needs to be onresult.as_ref()
to keep the underlying matrix alive, and then it's no problem if you get an&Mat
- you can just callMat::as_ref
on one of those, no trouble - that's just the regular API.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.
(see #12193 (comment))
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.
Yes, that works! Much better than duplicating the
matmul
call, thanks Jake!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.
Looking at the faer docs, it looks like you may be able to just do:
https://docs.rs/faer/latest/faer/#basic-usage
It obviously doesn't give us the same level as control as calling
faer::modules::core::mul::matmul;
but I wonder if there is a performance difference with it, as I think it would be a little cleaner looking than calling matmul like this.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.
Agreed, I think it looks cleaner too.
To test it, I ran the same tests as in the issue, and it seems like we lose performance if we don't call
faer::modules::core::mul::matmul;
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.
Wow, that's quite the difference, I wasn't expecting that. Then yeah lets leave it as 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.
Oh, wait what exactly were you timing here? Was it just the time to call
matmul
or did you include theclone()
time? The barematmul
call by itself is definitely gonna be faster, but you still need to allocate the result space which you are in effect doing with theclone()
call above. The multiplication operator just does that implicitly so it's not a manual operation.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.
Oh, yeah, I was only timing the
matmul
call in general, without using the same matrix as the destination either. I rewrote the test including theclone()
.So for the
matmul
call, I'm timing:For the other case:
ALPHA
andBETA
are 1 for the first case.