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

remove misleading bidiag constructor, make Bidiagonal immutable #22750

Merged
merged 1 commit into from
Jul 17, 2017

Conversation

fredrikekre
Copy link
Member

#22703 (comment)

Also changed Bidiagonal to an immutable, like Diagonal and [Sym]Tridiagonal

@@ -27,7 +27,6 @@ srand(1)
@test Bidiagonal(dv,ev,:U) != Bidiagonal(dv,ev,:L)
@test_throws ArgumentError Bidiagonal(dv,ev,:R)
@test_throws DimensionMismatch Bidiagonal(dv,ones(elty,n),:U)
@test_throws ArgumentError Bidiagonal(dv,ev)
Copy link
Contributor

Choose a reason for hiding this comment

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

change to MethodError instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, seems a bit ambitions to test a non-existing method though.

Copy link
Contributor

Choose a reason for hiding this comment

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

well it used to exist but only for the purpose of throwing an error, so if it accidentally were brought back (say an incorrect use of a default third argument?) that could be surprising

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

lgtm! Thanks @fredrikekre! :)

@fredrikekre
Copy link
Member Author

#22756 on macOS, tests relevant for this PR passed.

@KristofferC KristofferC merged commit e4bd638 into JuliaLang:master Jul 17, 2017
@fredrikekre fredrikekre deleted the fe/bidiagonal2 branch July 17, 2017 18:29
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.

4 participants