Skip to content

Conversation

youdymoo
Copy link
Contributor

@youdymoo youdymoo commented Apr 21, 2019

(Put an X inside the [ ] to denote check mark [X].)

  • If creating a new file:

    • added links to it in the README files?
    • included tests with it? (in tests/test_matrix.py)
    • added description (overview of algorithm, time and space complexity, and possible edge case) in docstrings?
  • if done some changes:

  • other

    • add valid unit tests of 7 methods of matrix (former coverage only 5.25%, now 49.86%)
    • follows the PEP8 code style

@coveralls
Copy link

coveralls commented Apr 21, 2019

Pull Request Test Coverage Report for Build 894

  • 47 of 48 (97.92%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+2.6%) to 74.763%

Changes Missing Coverage Covered Lines Changed/Added Lines %
algorithms/matrix/multiply.py 10 11 90.91%
Totals Coverage Status
Change from base Build 885: 2.6%
Covered Lines: 5590
Relevant Lines: 7477

💛 - Coveralls

7, 1, 3, 9, 2, 4, 8, 5, 6], [
9, 6, 1, 5, 3, 7, 2, 8, 4], [
2, 8, 7, 4, 1, 9, 6, 3, 5], [
3, 4, 5, 2, 8, 6, 1, 7, 9]]))
Copy link
Owner

@keon keon Apr 22, 2019

Choose a reason for hiding this comment

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

let's align the columns in the same level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. It's auto-formatted by the autopep8 LOL

return result


# def main():
Copy link
Owner

Choose a reason for hiding this comment

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

forgot to remove these?

@keon
Copy link
Owner

keon commented Apr 22, 2019

thanks for the contribution! this will be a nice addition to the matrix related algo.
i left some minor comments :)

@youdymoo
Copy link
Contributor Author

Just updated! Thanks for the review.

@keon keon merged commit 4d65694 into keon:master Apr 29, 2019
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.

Matrix multiplication
3 participants