-
Notifications
You must be signed in to change notification settings - Fork 6
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
WIP: Consistently use AbstractQuArray and define its interface. #29
Conversation
This is good stuff! What do you think of removing
We should definitely overload these when both arguments are of type
This a good example to play with. Assuming the implementation will look somewhat like:
...we would have to have a
So we'd have to define promotion for bases (which I believe could make sense). The tough part is coding up the solution to what I called The nice thing about using We'll definitely need conversion and promotion rules for the sake of genericness, though...otherwise, simple things like storing collections of In this same vein, how do we want to handle conversion/promotion between EDIT:: This gets even trickier if we try to get more generic with the
|
I like that. Done in one of the last commits.
Also done. I am still trying to figure out the promotion business. As you say, it would be nice to rely on the promotion rules for the coeff containers ... Another thing I realized only now is that |
That shouldn't be hard. From our side to base, conversion basically means calling |
Definitely, that's a good idea.
Yeah, this is basically what I was getting at - I didn't know whether or not we should have it, and if we did, how it would work (e.g. would |
Ok. Let's do the generalized CTranspose in a separate PR and open an issue for Array/QuArray promotion. Regarding this PR: I think the following behavior would be nice
Does that make sense? |
I think we basically have to rely on the container promotion, otherwise we will never get to do our actual project. But you are right, we should be careful with the promotions. I think your example actually reveals a bug (or an omission of defining the respective promote_rule)?! Regarding our promotions: Let's say we have a function
which takes advantage of our
which needs to be defined for each new subtype of
(for subtypes like For operations acting on two instances of the same subtype we can also make use of this idea
Mhm, actually this should also work if only the basis type is the same, but I don't know if one can enforce that ... That leaves the case with two different subtypes. For this we need the promotion. But if the stuff above works, we only need to get the primary type and the basis promotion right. The container type will be determined by
should do it. To make use of it we have to modify (I haven't tried your "fudging" approach, but it looks (hacky but) reasonable. The problem is that the promoted type might depend on the operation you do. I would rather like to rely on the inventor of |
I quite like this. Like
If you wanted to just enforce that the basis type is the same, the signature would be I think this idea works great, though, if I understand you correctly. Slightly modifying it, we could do:
...where I think the above modified version of This is a cool approach! |
Even better! I will try your suggestion and see how it works ... |
3e13212
to
f04508b
Compare
Seems to work nicely. Once #31 has landed we will have to revisit all those I also fixed the deprecation warnings on 0.4-dev. For this I had to mess with labelbasis.jl; it would be good if you could check that. |
Dunno. The warning said "replace int(...) by round(Int, ...)". EDIT: But |
That's because |
Alright, I will change that. What else should be done in this PR? Shall I add a new type to test the promotion etc? |
|
||
# matrix operations returning an array | ||
# sparse to dense | ||
function Base.full(qarr::AbstractQuMatrix) |
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.
Could/should we have a macro for this construct (like @delegate
you mentioned once)? This code pattern is probably quite common and having a macro would be nice for users to turn their favorite matrix function into a QuMatrix function.
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.
So like @applycoeffs f(qa)
--> QuArray(f(coeffs(qa)), bases(qa))
? That could work. OTOH, making users write out QuArray(f(coeffs(qa), bases(qa))
might result in more "deliberate" code, as the intent about the return type of f
is clearer. I could go either way, honestly.
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.
Ok. Then we leave it like that for now. It's more of a convenience feature anyways, which we can also do later if we want.
I did some cleanup and added a few missing exports. There is currently a ambiguity warning for Apart from that it seems to work. I defined
and so far it behaved as expected (e.g. returns a I would say if everyone is happy (and we figured out the ambiguity warning), we merge this and add the new types in a separate PR ... |
@jrevels : Thanks! Looks like your suggestion does the job. Is there anything else that should be done? |
lower_single(n) = sparse([1:n-1], [2:n], laddercoeffs(n-1), n, n) | ||
raise_single(n) = sparse([2:n], [1:n-1], laddercoeffs(n-1), n, n) | ||
lower_single(n) = sparse([1:n-1;], [2:n;], laddercoeffs(n-1), n, n) | ||
raise_single(n) = sparse([2:n;], [1:n-1;], laddercoeffs(n-1), n, n) |
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.
Why the addition of the semicolons here?
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.
This is needed for 0.4. I think it has to do with concatenating vs non-concatenating behavior.
EDIT: For instance
julia> [1:3]
WARNING: [a] concatenation is deprecated; use [a;] instead
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.
Ah, okay. I think this PR is ready to merge then :)
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.
Cool :-) Then let's do it and see how it works in the real world ... Just one more question: what shall we do about typerepr
? Should every type provide one or do we want a default typerepr(::AbstractQuArray)
?
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'd be cool with a default that looked like:
typerepr{Q<:AbstractQuArray}(::Q) = "$Q"
...and then overload it for specific types when appropriate.
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.
Ok, I will add this and then merge ...
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.
In the end I used typerepr(qa::AbstractQuArray) = "$(typeof(qa).name)"
, which was easier to add and gives nice output (could be that we don't need the other typerepr
at all).
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, nice! I didn't know about the name
field. In that case, yeah, typerepr
can be removed. If you want to remove it now before merging you can, otherwise I'll do so later.
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 learned about it only by accident. Seems that the more individual names are nicer (e.g. QuVector
is only printed as QuArray
). So let's leave it like that then.
…s on AbstractQuArrays.
Consistently use AbstractQuArray and define its interface in terms of functions rawcoeffs, rawbases, coefftype, similar_type and copy. Fix deprecation warnings for 0.4-dev.
Let's move on ... |
This is a first stab at using
AbstractQuArray
instead ofUnion(QuArray,CTranspose)
whenever possible. The interface for anAbstractQuArray
is so-far basically defined by the functionsrawcoeffs
,coeffscoefftype
rawbases
,basescopy
similar_type
(see comments for a definition)One of the open issues is type promotion of different
AbstractQuArray
s for example in arithmetic operations. Right now+
and-
always returns aQuArray
, which is probably not desirable. Somewhat related is the question of how we want to implement new subtypes ofAbstractQuArray
(for example density matrices). Any ideas and suggestions are welcome.