Skip to content

Added OOP approach to matrices #1165

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

Merged
merged 7 commits into from
Sep 8, 2019
Merged

Added OOP approach to matrices #1165

merged 7 commits into from
Sep 8, 2019

Conversation

jasper256
Copy link
Contributor

This is far from done but just a little project I was working on for fun that I thought would be nice to add to this repo eventually. I still need to add determinants, cofactors, etc. but I thought it might be good to post this and see if I got any advice / criticisms of things to think or if you guys thought it looked good to keep going the way it is. I just noticed that you guys are now recommending running Python Black, which I haven't done yet but will get to soon. Anyway, let me know what you think!

…onding doctests) to matrix_class.py

A small bug persists that causes the doctest to fail.
After a couple Matrix objects are printed, the next one is printed in a different format.
@jasper256
Copy link
Contributor Author

I have now added all methods I intended to, but can add more if people have suggestions. I also formatted with python/black as recommended. One mild bug that persists is the issue of printing out Matrix objects. For some reason, after a couple (I believe it's two, but it might vary) are printed, the next one to be printed is formatted incorrectly. This causes the doctest to fail in one case. I don't believe this is an error with the str method because when playing around in IDLE I found that I can get any Matrix to print correctly, but when I print a few in a row the bug occurs. Please let me know if anyone knows how to fix this, as well as any general feedback.

@jasper256 jasper256 changed the title Added OOP aproach to matrices Added OOP approach to matrices Sep 2, 2019
…ests in matrix_class.py. Also implemented eq and ne comparison operations
@McDic
Copy link
Contributor

McDic commented Sep 3, 2019

  1. pow method is linear complexity, you can reduce it to logarithm complexity with simple optimizations.

  2. Why did you applied property decorator to determinant, minors, etc while their time complexity is non-polynomial? Wouldn't be better to consider these methods as non-properties so developers will think calculations are expensive?

  3. It would be better if str method generates numpy/scipy style.

@cclauss
Copy link
Member

cclauss commented Sep 3, 2019

@jasper256
Copy link
Contributor Author

jasper256 commented Sep 3, 2019

Thanks for the feedback.

1. `pow` method is linear complexity, you can reduce it to logarithm complexity with simple optimizations.

I am not especially knowledgeable in this subject area so I will look into that and fix it as soon as possible.

2. Why did you applied `property` decorator to `determinant`, `minors`, etc while their time complexity is non-polynomial? Wouldn't be better to consider these methods as non-properties so developers will think calculations are expensive?

I am a pretty inexperienced programmer so I don't really have a good reason as to 'why'. I didn't realize that this indicated polynomial time complexity (I barely understand time complexity but am looking to learn more about it). Anyway, I will certainly change this. I think I will leave num_rows, num_columns, order, and is_square as properties because they are much less complex unless you think they should be changed too. Let me know.

3. It would be better if `str` method generates numpy/scipy style.

I had never used numpy before so I didn't even realize this standard existed. Thanks for bringing it to my attention, I made this change.

@McDic
Copy link
Contributor

McDic commented Sep 4, 2019

Try this at the end of the code. Your program will never terminate if given input is larger than 4.

if __name__ == "__main__":
    pass
    # import doctest
    # test = doctest.testmod()

    import random
    size = int(input("Input size: "))
    mat = [[random.random() * 100 for j in range(1, size+1)] for i in range(size)]
    mat = Matrix(mat)
    print(mat.determinant())

I think you need more optimizations.

@McDic
Copy link
Contributor

McDic commented Sep 4, 2019

And I meant clean string style for __str__ is making same length for each row.

Your initial commit shows strings like

[1, 2, 3]
[11, 22, 33]
[17, 6, -2]

and I intended

[ 1,  2,  3]
[11, 22, 33]
[17,  6, -2]

But I don't think fixing __str__ is not important, as there is some critical performance issue on determinant, inversing, etc.

@jasper256
Copy link
Contributor Author

@McDic I just wanted to say that I've seen your comments and appreciate the feedback. Sorry if it is taking me a while to make changes but school just started for me and I have been very busy. So this might take me a while to complete, but don't think I have forgotten about it.

@cclauss
Copy link
Member

cclauss commented Sep 8, 2019

This is in a useful state and passes the tests so let’s land and the improvements requested above can come as pull requests.

@cclauss cclauss merged commit 5b483be into TheAlgorithms:master Sep 8, 2019
stokhos pushed a commit to stokhos/Python that referenced this pull request Jan 3, 2021
* Added OOP aproach to matrices

* created methods for minors, cofactors, and determinants and added corresponding doctests

* Added methods for adjugate, inverse, and identity (along with corresponding doctests) to matrix_class.py
A small bug persists that causes the doctest to fail.
After a couple Matrix objects are printed, the next one is printed in a different format.

* formatted matrix_class.py with python/black

* implemented negation and exponentiation as well as corresponding doctests in matrix_class.py.  Also implemented eq and ne comparison operations

* changed __str__ method in matrix_class.py to align with numpy standard and fixed bug in cofactors method

* removed property decorators from several methods in matrix_class.py
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.

3 participants