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

Add solver for distributed triangular systems #1236

Merged
merged 36 commits into from
Apr 12, 2024

Conversation

mrfh92
Copy link
Collaborator

@mrfh92 mrfh92 commented Oct 10, 2023

This PR is intended to add a triangular solver (see, e.g., torch.linalg.solve_triangular) to Heat; see #1096.

Due Diligence

  • General:
    • base branch must be main for new features, latest release branch (e.g. release/1.3.x) for bug fixes
    • title of the PR is suitable to appear in the Release Notes
  • Implementation:
    • unit tests: all split configurations tested
    • unit tests: multiple dtypes tested
    • documentation updated where needed

…ll cases covered, may be inefficient, no tests so far!)
@ghost
Copy link

ghost commented Oct 10, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@mrfh92
Copy link
Collaborator Author

mrfh92 commented Oct 10, 2023

Things to be addressed:

  • all combinations of splits for A and b; b either vector or matrix
  • performance (scaling)
  • unit tests to be added

@github-actions
Copy link
Contributor

Thank you for the PR!

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: Patch coverage is 90.10989% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 92.04%. Comparing base (6e9e9ec) to head (6589754).
Report is 1 commits behind head on main.

Files Patch % Lines
heat/core/linalg/solver.py 90.10% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1236      +/-   ##
==========================================
- Coverage   92.06%   92.04%   -0.02%     
==========================================
  Files          80       80              
  Lines       11941    12031      +90     
==========================================
+ Hits        10993    11074      +81     
- Misses        948      957       +9     
Flag Coverage Δ
unit 92.04% <90.10%> (-0.02%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link
Contributor

Thank you for the PR!

@github-actions
Copy link
Contributor

Thank you for the PR!

@mrfh92 mrfh92 mentioned this pull request Oct 12, 2023
5 tasks
Copy link
Contributor

This pull request is stale because it has been open for 60 days with no activity.

@github-actions github-actions bot added the stale label Dec 18, 2023
Copy link
Contributor

Thank you for the PR!

@mrfh92
Copy link
Collaborator Author

mrfh92 commented Dec 18, 2023

Things to be done:

  • which cases have been covered so far? (which combinations of A.split $\in$ {0, 1,, None}, b.split $\in$ {0, 1,, None})
  • tests need to be added for the cases covered so far
  • does extension to the cases not covered so far make sense (from the algorithmic point of view)?

@github-actions github-actions bot removed the stale label Dec 25, 2023
@FOsterfeld FOsterfeld self-assigned this Jan 2, 2024
Copy link
Contributor

github-actions bot commented Jan 3, 2024

Thank you for the PR!

@mrfh92
Copy link
Collaborator Author

mrfh92 commented Jan 10, 2024

With a view on #1104 #1320 it might also be usefull to include the case of batched inputs (however, for the moment it should suffice to cover the simple case split=batch-dimension)

Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

@mrfh92
Copy link
Collaborator Author

mrfh92 commented Jan 22, 2024

There seems to be an error that is not due to the (meanwhile) resolved QR errors etc.

Copy link
Contributor

github-actions bot commented Apr 3, 2024

Thank you for the PR!

Copy link
Contributor

Thank you for the PR!

Copy link
Contributor

@ClaudiaComito ClaudiaComito left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the changes @mrfh92 @FOsterfeld , looks great, I only have a few more editing suggestions.

heat/core/linalg/solver.py Outdated Show resolved Hide resolved
heat/core/linalg/solver.py Outdated Show resolved Hide resolved
heat/core/linalg/solver.py Outdated Show resolved Hide resolved
heat/core/linalg/solver.py Outdated Show resolved Hide resolved
heat/core/linalg/solver.py Outdated Show resolved Hide resolved
mrfh92 and others added 3 commits April 11, 2024 11:01
Co-authored-by: Claudia Comito <39374113+ClaudiaComito@users.noreply.github.com>
Co-authored-by: Claudia Comito <39374113+ClaudiaComito@users.noreply.github.com>
Co-authored-by: Claudia Comito <39374113+ClaudiaComito@users.noreply.github.com>
Copy link
Contributor

Thank you for the PR!

1 similar comment
Copy link
Contributor

Thank you for the PR!

incorporated @ClaudiaComito's suggestions for docstring
@mrfh92
Copy link
Collaborator Author

mrfh92 commented Apr 11, 2024

@ClaudiaComito I have taken your review into account now.

Copy link
Contributor

Thank you for the PR!

1 similar comment
Copy link
Contributor

Thank you for the PR!

@ClaudiaComito ClaudiaComito added this to the 1.4.0 milestone Apr 12, 2024
@ClaudiaComito ClaudiaComito changed the title Add solver for triangular systems Add solver for distributed triangular systems Apr 12, 2024
Copy link
Contributor

@ClaudiaComito ClaudiaComito left a comment

Choose a reason for hiding this comment

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

Thanks a lot @FOsterfeld , @mrfh92 !

Copy link
Contributor

Thank you for the PR!

@mrfh92 mrfh92 merged commit b070199 into main Apr 12, 2024
10 checks passed
@mrfh92 mrfh92 deleted the features/1096-Provide_a_solver_for_triangular_systems branch April 12, 2024 13:46
@mrfh92 mrfh92 restored the features/1096-Provide_a_solver_for_triangular_systems branch April 15, 2024 10:40
FOsterfeld added a commit that referenced this pull request Jun 1, 2024
ClaudiaComito added a commit that referenced this pull request Jun 12, 2024
* solve_triangular from previous pr #1236

* added test for type and value errors

---------

Co-authored-by: Claudia Comito <39374113+ClaudiaComito@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants