Skip to content

fixing issue: Determinant function raises TypeError on simple matrix #542#543

Closed
sonntagsgesicht wants to merge 7 commits intompmath:masterfrom
sonntagsgesicht:master
Closed

fixing issue: Determinant function raises TypeError on simple matrix #542#543
sonntagsgesicht wants to merge 7 commits intompmath:masterfrom
sonntagsgesicht:master

Conversation

@sonntagsgesicht
Copy link
Contributor

No description provided.

@fredrik-johansson
Copy link
Collaborator

The solution looks like a bit of a hack, and it's problematic that it doesn't account for the scaling of the matrix. For example, it will return 0 if you input the identity matrix times 1e-30. It would be fine if you check any column for being exactly zero, but perhaps that isn't general enough.

@sonntagsgesicht
Copy link
Contributor Author

sonntagsgesicht commented Jun 22, 2020

Yes, you are right. Had a closer look and fixed it directly inside LU_comp where pivoting failed.
Added a LU_comp test, too. But, since I have no pytest experience on testing exceptions, this test looks more like a hack, too.

@mgmax
Copy link

mgmax commented Sep 29, 2020

If you don't like the compact pytest.raises(Exception, function) notation, you can also use pytest.raises like this:

    with pytest.raises(MyException):
        do_something_wrong()

This checks that the expected exception is thrown somewhere within the "with"-block, and is actually the recommended notation in the pytest docs.

@vks
Copy link
Contributor

vks commented Feb 4, 2021

The updated fix looks good to me.

@sonntagsgesicht sonntagsgesicht deleted the branch mpmath:master September 16, 2021 06:45
@sonntagsgesicht sonntagsgesicht deleted the master branch September 16, 2021 06:45
skirpichev added a commit that referenced this pull request Aug 6, 2023
(Update on PR #543) Fixed TypeError in `LU_comp`, updated `determinant` docs and added `rank` function for matrices
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