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

Simpler CTranspose + multiplication methods #14

Merged
merged 9 commits into from
Mar 3, 2015
Merged

Simpler CTranspose + multiplication methods #14

merged 9 commits into from
Mar 3, 2015

Conversation

jrevels
Copy link
Contributor

@jrevels jrevels commented Mar 1, 2015

Okay, hopefully this will be the last attempt needed to kill #9 so we can finally move forward!

This PR defines bunch of multiplication methods that are consistent, using @acroy's method to resolve the row/column vector issues. It also adds actual tests for these methods. They're not exactly comprehensive, but it's a start.

Actually defining the multiplication methods made me realize how messy having separate Conjugate/Transpose types makes things, so I simply removed those types from the picture. If users wish to separately conjugate/transpose coefficient arrays, we'll let them worry about that (or just do so eagerly).

Thoughts?

@jrevels
Copy link
Contributor Author

jrevels commented Mar 1, 2015

Also it should be noted that, in the future, we could set up type promotion for basis types that would allow us to expand beyond forcing both arguments to have the same basis type.

##################
# bra * ket -> scalar
function *{B}(bra::DualVector{B}, ket::QuVector{B})
return first(Ac_mul_B(rawcoeffs(bra), rawcoeffs(ket)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use dot here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@acroy
Copy link
Contributor

acroy commented Mar 1, 2015

Thanks for putting this together. Looks good at a first glance. I will try it out tomorrow. You forgot to call runtests.jl in travis.yml. We probably have to activate Travis for this repo as well.

@acroy
Copy link
Contributor

acroy commented Mar 2, 2015

Ok, Travis should work with the next push/PR (forget the comment about runtests.jl). I have also added a status banner to README.

Otherwise the PR looks good. There are just two things we should keep in mind (and maybe put a comment in the code): 1) Currently we ignore the inner product of the basis functions, i.e., assume an orthonormal basis. 2) The multiplication doesn't work for N>2. We probably want to extend this at least to N=4 (super-operators), but ideally want it to work in general (or at least make it easy to extend).

@acroy
Copy link
Contributor

acroy commented Mar 2, 2015

I don't know why Travis complains...

@jrevels
Copy link
Contributor Author

jrevels commented Mar 2, 2015

Did it complain? Everything's turning up green as far as I can see, but I'm not super familiar with Travis so I'm probably missing something. Anyway, thanks for setting it up.

Agreed on the generality points you made. I think for basis products we're going to want to define the functions inner{A,B,S}(a::NTuple{A,FiniteBasis{S}}, b::NTuple{B,FiniteBasis{S}}) and
tensor{A,B,S}(a::NTuple{A,FiniteBasis{S}}, b::NTuple{B,FiniteBasis{S}}), then use those to construct more general multiplication definitions.

Just to be on the safe side, I'll restrict the functions defined here to S<:Orthonormal in the meantime.

Edit: The page just refreshed as I posted this, see the failure now. I think I just need to pull master into the dualmult branch to update the travis file and it should be fine.

@acroy
Copy link
Contributor

acroy commented Mar 2, 2015

Now it looks good (I.e. green). It was probably just the old yml file.

jrevels added a commit that referenced this pull request Mar 3, 2015
Simpler CTranspose + multiplication methods
@jrevels jrevels merged commit aec7e2f into master Mar 3, 2015
@jrevels jrevels deleted the dualmult branch March 13, 2015 17:17
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