Skip to content

Conversation

@andreasnoack
Copy link
Member

For now, it requires some type asserts to achieve this. Hopefully, they'll be unnecessary at some point.

@andreasnoack
Copy link
Member Author

As discussed in #11240

@timholy
Copy link
Member

timholy commented May 13, 2015

LGTM. Since the type of F[:X] depends on the value of X, I don't think there is any way to make this type stable without the asserts (unless we use Val for this, too).

@andreasnoack
Copy link
Member Author

That's right, but we never know what the future will bring to Julia.

@tkelman
Copy link
Contributor

tkelman commented May 13, 2015

Some of the type asserts are failing on the integer type https://travis-ci.org/JuliaLang/julia/jobs/62418292, and there are some other scary-looking failures that have recently started showing up in a few other builds too...

@daviddelaat
Copy link
Contributor

I'm sure this has been discussed somewhere (and maybe its off-topic here), but do you know why indexing with symbols is used instead of functions to get the factors? I would think that something like qfactor(F), rfactor(F), pivotvector(F), and pivotmatrix(F) looks fine and is easier to implement efficiently.

@andreasnoack
Copy link
Member Author

Initially, we underestimated the importance of type stability in the factorization methods, e.g. we moved from qrpfact to using a keyword argument which has recently changed to using Val to regain type stability. Another problem is that it is not satisfactory to have so many different extractor functions. It is not really generic or dispatchy enough for Julia.

I believe that we'll be able to make the extraction of factors type stable with an approach very similar to what we are doing now. I'm very interested in this and eventually we'll find some type stable solution to this.

@andreasnoack
Copy link
Member Author

@tkelman Thanks for pointing out the integer type error. It was 32 bit related and is now fixed. The other error was unrelated so I'll merge.

andreasnoack added a commit that referenced this pull request May 15, 2015
@andreasnoack andreasnoack merged commit c47c326 into master May 15, 2015
@andreasnoack andreasnoack deleted the anj/qr branch May 15, 2015 00:41
@daviddelaat
Copy link
Contributor

Thanks for the explanation Andreas!

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.

5 participants