Open
Conversation
Author
|
Oh yeah, btw this isn't related to the code, but I use dark mode on github, and you can barely see the image, and the python's a bit off-putting with the black eyes so if you see this could you also change that? Thanks :D |
Author
|
Updated the pull request's explanation and code cause I accidentally entered and tried to explain the wrong version lol |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Algorithm's Issue
When trying to implement the matrix multiplication algorithm in this library (getting the REF (Row Echelon Form) with Gaussian Elimination and then multiplying the main diagonal for the determinant) I found the issue with
determinant_fast()that was also mentioned by issue #2.The pivot being 0 would cause
determinant_fast()to return the wrong determinant due to the lack of row swapping.The Fix
This pull request fixes that by adding row swapping like this on line 331:
alongside the
odd_swapsvariable for check whether there's been an odd number of swaps and the negator at the end for if there has been an odd number of swaps.and removing the "cheat":
from here
The Mistake
Contrary to what's said in the article:
the cheat and fancy system that includes keeping track of the number of times rows were swapped to change the sign at the end isn't required, all that's required is row swapping and a boolean to tell whether or not there's been an odd number of swaps.
Please update the article
https://integratedmlai.com/find-the-determinant-of-a-matrix-with-pure-python-without-numpy-or-scipy/
The Solution for those who are trying to use the algorithm
The author doesn't appear to be active on his github, so hopefully this'll help anyone who's confused
just copy
determinant_fast()from my pull request here: https://github.com/RandomGamingDev/BasicLinearAlgebraToolsPurePy/blob/master/LinearAlgebraPurePython.py#L315How can I contact @ThomIves
Also, I don't know how to contact the author so it'd be appreciated if someone who did know could help contact him or send his contact details to me so that we can update the article.