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

Added Normalize to QuArray. #15

Merged
merged 1 commit into from
Mar 9, 2015
Merged

Conversation

amitjamadagni
Copy link
Contributor

This is my first attempt at contributing. Though not that significant, please do comment. Thanks.

@acroy
Copy link
Contributor

acroy commented Mar 5, 2015

Thanks for your contribution! It's a good start. Accidentally I thought that adding norms would be a natural next step ...

Regarding your code I have some comments (mostly because we don't have an explicit guide how such functions for QuArrays should work, but I have many implicit ideas ;-)

  • Firstly, norms are not so easy. You used Base.norm on the container, which computes the "p-norm". However, for (Hilbert space) operators it is more common to use the Frobenius norm, which you would get via Base.vecnorm. For vectors this doesn't matter, but for N>1 we should probably use the latter. Note that we need a different norm (i.e. trace) for density matrices though.
  • I would prefer to have a separate function norm(qa::QuArray) (and probably also for CTranspose).
  • For scaling we should provide a mutating function by extending Base.scale! and then implement multiplication and division in terms of that. You will need a copy function first (@jrevels Could you add copy for QuArray and CTranspose?).
  • Note that your current normalize! function actually makes a copy of the coefficient array.
  • I think coeffs was replaced by rawcoeffs in one of the latest commit?
  • Now that we have Travis in place, which runs tests for each PR, we should really try to add tests for each function/operation we add.

Sorry for the long list of comments. Maybe the more general points should go into some "guideline issue".

@amitjamadagni
Copy link
Contributor Author

@acroy I have worked on the commit. Here are the details :

  • Attempt at implementing the norm function which takes QuArray as a parameter.
  • Attempt at implementing copy for QuArray and CTranspose.
  • normalize, normalize! implemented.

WIP :

  • Scaling
  • Tests

Doubts :

  1. Requirement of rawcoeffs when .coeffs is doing the same job ??


function norm(qarr::QuArray)
if ndims(rawcoeffs(qarr)) == 1
return norm(rawcoeffs(qarr))
Copy link
Contributor

Choose a reason for hiding this comment

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

You can always use vecnorm. For N=1 this is the same as norm (sum of absolute values squared of all elements and square root of that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad !!! Sorry for this. Done editing.

@acroy
Copy link
Contributor

acroy commented Mar 6, 2015

Nice! The point of using rawcoeffs is that you can use it for QuArrays and CTranspose, i.e., norm could take qa::Union(QuArray,CTranspose) and you can simply call rawcoeffs(qa).

return QuArray(num*qarr.coeffs, qarr.bases)
end

function norm(qarr::QuArray)
Copy link
Contributor

Choose a reason for hiding this comment

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

You have to use function Base.norm or import Base: norm otherwise norm will only be visible within the module.

@acroy
Copy link
Contributor

acroy commented Mar 7, 2015

If you use rawcoeffs and bases instead of accessing the fields (qa.coeffs) you can replace the arguments qa::QuArray by qa::Union(QuArray,CTranspose) (I think). Don't forget to add norm to the import at the top of arraymath.jl. For the test you can basically use the same approach as Jarrett.

@amitjamadagni amitjamadagni force-pushed the normalize branch 2 times, most recently from e23cc47 to 97364c1 Compare March 7, 2015 22:54
@amitjamadagni
Copy link
Contributor Author

@acroy is this good to go ?? Any comments would be helpful.

@@ -1,12 +1,13 @@
import Base: *
import Base: norm, scale, scale!
Copy link
Contributor

Choose a reason for hiding this comment

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

Those imports you can just add to the line above.

@acroy
Copy link
Contributor

acroy commented Mar 9, 2015

Looks good now. Thanks! Just one minor point: could you squash your commits into a single one? It's not really necessary but makes the history more readable.

@amitjamadagni
Copy link
Contributor Author

@acroy done. Thanks for all the help.

acroy added a commit that referenced this pull request Mar 9, 2015
Add norm, normalize(!), copy and scale(!) for QuArrays including tests.
@acroy acroy merged commit 8126351 into JuliaAttic:master Mar 9, 2015
@amitjamadagni amitjamadagni deleted the normalize branch June 16, 2015 06: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