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

RFC: Fix behaviour of sqrtm on UpperTriangular matrices #12570

Merged
merged 1 commit into from
Aug 12, 2015

Conversation

mfasi
Copy link
Contributor

@mfasi mfasi commented Aug 11, 2015

sqrtm does not automatically promote the work matrix to a complex type if any of the elements on the diagonal is negative. An example of the issue is

A = UpperTriangular([-1 2; 0 3.]); sqrtm(A)

@kshyatt kshyatt added the linear algebra Linear algebra label Aug 11, 2015
@andreasnoack
Copy link
Member

Could you also add a test for this?

@mfasi mfasi force-pushed the sqrtm_working_type branch from 98ab148 to a528c23 Compare August 12, 2015 09:46
@mfasi
Copy link
Contributor Author

mfasi commented Aug 12, 2015

@andreasnoack Thank you, I added a few tests which should achieve complete line coverage (of the sqrtm functions).

@andreasnoack
Copy link
Member

Thanks. Just a small detail. We are moving towards using instead of test_approx_eq so would you change the tests to something like @test sqrtm(Atn) |> t->t*t ≈ Atn.

@mfasi mfasi force-pushed the sqrtm_working_type branch from a528c23 to 8527eb6 Compare August 12, 2015 13:21
@mfasi
Copy link
Contributor Author

mfasi commented Aug 12, 2015

I tried to use but it does not seem to be defined. Should I import something in the test file? The symbol I'm using should be U+2248

@andreasnoack
Copy link
Member

which version of Julia are you using?

@mfasi
Copy link
Contributor Author

mfasi commented Aug 12, 2015

My current version was:

Version 0.4.0-dev+6561 (2015-08-09 23:36 UTC)
Commit 5505cf8* (2 days old master)

Now I am rebasing the branch on master, but I doubt things will change -- unless this symbol has been there for less than 2 days. Anyway, it really looks like it's just an issue of mine, the tests apparently work fine on AppVeyor and Travis.

@yuyichao
Copy link
Contributor

unless this symbol has been there for less than 2 days

This is exactly what happend. #12472

@mfasi
Copy link
Contributor Author

mfasi commented Aug 12, 2015

@yuyichao Thank you, it works now.

andreasnoack added a commit that referenced this pull request Aug 12, 2015
RFC: Fix behaviour of sqrtm on UpperTriangular matrices
@andreasnoack andreasnoack merged commit d1a681b into JuliaLang:master Aug 12, 2015
@andreasnoack
Copy link
Member

Great. Thanks for fixing this.

@mfasi mfasi deleted the sqrtm_working_type branch August 12, 2015 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants