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

Feature/337 determinant #877

Merged
merged 15 commits into from
Jan 24, 2022
Merged

Feature/337 determinant #877

merged 15 commits into from
Jan 24, 2022

Conversation

mtar
Copy link
Collaborator

@mtar mtar commented Oct 4, 2021

Description

First implementation of determinant computation using Gaussian elimination with partial pivoting.

Issue/s resolved: #337

Changes proposed:

  • add linalg.det

Type of change

  • New feature

Due Diligence

  • All split configurations tested
  • Multiple dtypes tested in relevant functions
  • Documentation updated (if needed)
  • Updated changelog.md under the title "Pending Additions"

Does this change modify the behaviour of other functions? If so, which?

no

@codecov
Copy link

codecov bot commented Oct 4, 2021

Codecov Report

Merging #877 (593ad7f) into master (293d873) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #877      +/-   ##
==========================================
+ Coverage   95.50%   95.52%   +0.02%     
==========================================
  Files          64       64              
  Lines        9579     9617      +38     
==========================================
+ Hits         9148     9187      +39     
+ Misses        431      430       -1     
Flag Coverage Δ
gpu 94.66% <100.00%> (+0.03%) ⬆️
unit 91.10% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
heat/core/dndarray.py 97.00% <ø> (ø)
heat/core/linalg/basics.py 95.26% <100.00%> (+0.21%) ⬆️
heat/core/types.py 96.84% <0.00%> (+0.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 293d873...593ad7f. Read the comment docs.

@mtar mtar linked an issue Oct 4, 2021 that may be closed by this pull request
Base automatically changed from bug/866-op-nested to master October 8, 2021 09:55
@mtar mtar marked this pull request as ready for review October 11, 2021 13:41
@ClaudiaComito ClaudiaComito added this to the 1.2.x milestone Oct 27, 2021
Copy link
Member

@coquelin77 coquelin77 left a comment

Choose a reason for hiding this comment

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

looks great to me. Just a small change

heat/core/linalg/basics.py Outdated Show resolved Hide resolved
@coquelin77
Copy link
Member

@mtar i have changed the shape rules to line up with torch's results. can you check this again?

@mtar
Copy link
Collaborator Author

mtar commented Jan 12, 2022

@coquelin77 Thanks for addressing the wrong dimension of the result tensor.

I was now able to reduce some lines of code due to a fix in master.

heat/core/linalg/basics.py Outdated Show resolved Hide resolved
@coquelin77 coquelin77 merged commit 2867fe9 into master Jan 24, 2022
@coquelin77 coquelin77 deleted the feature/337-determinant branch January 24, 2022 07:22
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.

Impelement matrix determinant
3 participants