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

(Update on PR #543) Fixed TypeError in LU_comp, updated determinant docs and added rank function for matrices #610

Merged
merged 6 commits into from
Aug 6, 2023

Conversation

jan-philipp-hoffmann
Copy link

This is an update on PR #543

  • fixed an issue in LU_comp which raised TypeError on singular matrices
  • added documentation of determinant function of matrices
  • added rank function of matrices

(PR #543 was originally opened and closed by sonntagsgesicht)

@skirpichev skirpichev self-assigned this Apr 20, 2023
@skirpichev skirpichev closed this May 11, 2023
@skirpichev skirpichev reopened this May 11, 2023
@skirpichev
Copy link
Collaborator

There are test failures.

@jan-philipp-hoffmann
Copy link
Author

minor changes to fixe test failures. tests should work now (as you can check here).

@jan-philipp-hoffmann
Copy link
Author

sorry, another fix needed in docs. locally I can build docs without failure.

Copy link
Collaborator

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

Looks good, except for minor nitpicks below.

Could you, please, also do a rebase? Probably, there should be four commits:

  • update on .gitignore
  • issue in LU_comp (with a test)
  • det docs
  • rank function (with tests)

mpmath/matrices/linalg.py Show resolved Hide resolved
mpmath/matrices/linalg.py Outdated Show resolved Hide resolved
mpmath/matrices/linalg.py Outdated Show resolved Hide resolved
mpmath/matrices/linalg.py Outdated Show resolved Hide resolved
@skirpichev skirpichev requested a review from vks May 17, 2023 06:12
@skirpichev skirpichev removed their assignment May 17, 2023
mpmath/matrices/linalg.py Outdated Show resolved Hide resolved
@@ -134,6 +134,9 @@ def LU_decomp(ctx, A, overwrite=False, use_cache=True):
if current > biggest: # TODO: what if equal?
biggest = current
p[j] = k
# without pivot LU fails
if p[j] is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this what happens for the test case A11?

Choose a reason for hiding this comment

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

yes

columns (or rows equivalently).

Rank is computed via singular value decomposition
by counting the number of non-zero singular values.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please document the iszerofunc parameter, and maybe add an example using it?

Choose a reason for hiding this comment

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

added tests some example for iszerofunc

Copy link
Contributor

@vks vks left a comment

Choose a reason for hiding this comment

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

Looks good from my side, I only had minor additional comments. I agree with @skirpichev's comments.

update imports in test_linalg.py

custom `iszerofunc` in rank determination

adding rank of matrix via counting eigenvalues form svd_r
@skirpichev skirpichev requested a review from vks June 2, 2023 07:31
@skirpichev skirpichev merged commit 91ccdd4 into mpmath:master Aug 6, 2023
7 checks passed
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.

None yet

4 participants