Skip to content

Allow rank computation for QRPivoted matrices #54283

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 30, 2024
Merged

Allow rank computation for QRPivoted matrices #54283

merged 6 commits into from
Apr 30, 2024

Conversation

@danilonumeroso
Copy link
Contributor

@danilonumeroso danilonumeroso commented Apr 27, 2024 •
edited
Loading

Hi, this fixes JuliaLang/LinearAlgebra.jl#1052. I followed @stevengj's code suggestion which worked just fine and added tests in the related section.

Note: this is my first contribution so please feel free to point out any rookie mistakes I might have made!

@jishnub jishnub added the linear algebra Linear algebra label Apr 27, 2024
@stevengj
Copy link
Member

stevengj commented Apr 27, 2024

LGTM. Do you also want to tackle JuliaLang/LinearAlgebra.jl#1052? If nullspace and cond aren't addressed in this PR, which is fine, we should open a new issue for those after this PR is merged.

@stevengj stevengj added the needs news A NEWS entry is required for this change label Apr 27, 2024
@stevengj
Copy link
Member

stevengj commented Apr 27, 2024 •
edited
Loading

Probably worth adding a NEWS.md entry.

Not sure where else it would be good to document this?

@danilonumeroso
Copy link
Contributor Author

danilonumeroso commented Apr 28, 2024

LGTM. Do you also want to tackle #53214 (comment)? If nullspace and cond aren't addressed in this PR, which is fine, we should open a new issue for those after this PR is merged.

Yes, I'd like to. I'm going to open a new issue for those and tackle it in a separate PR

Probably worth adding a NEWS.md entry.

Not sure where else it would be good to document this?

Done! Please tell me if I need to be more specific and/or there's any standard I should follow for NEWS.md entries

barucden
barucden reviewed Apr 28, 2024
View reviewed changes
@LilithHafner LilithHafner added merge me PR is reviewed. Merge when all tests are passing and removed needs news A NEWS entry is required for this change labels Apr 28, 2024
stevengj
stevengj reviewed Apr 28, 2024
View reviewed changes
@fatteneder fatteneder merged commit 831ebe0 into JuliaLang:master Apr 30, 2024
@fatteneder fatteneder removed the merge me PR is reviewed. Merge when all tests are passing label Apr 30, 2024
@LilithHafner LilithHafner mentioned this pull request Oct 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevengj stevengj stevengj approved these changes

+1 more reviewer

@barucden barucden barucden left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

linear algebra Linear algebra

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

rank(::QRPivoted) method?

6 participants

@danilonumeroso @stevengj @barucden @jishnub @fatteneder @LilithHafner