Refactoring of QR: stabilized Gram-Schmidt for split=1 and TS-QR for split=0 #1329

mrfh92 · 2024-01-22T14:26:44Z

Description

Changes in the implementation of QR:

split=1: column-block-wise stabilized/modified Gram Schmidt; thus any shape allowed.
split=0: TS-QR ("tall skinny QR"); thus input must be tall-skinny in the sense that each local chunk of data has not less rows than columns. Removing this restriction can be considered as future work.
API is now more NumPy/PyTorch-like: inputs are the input matrix and a string mode; the latter can be reduced and r, but we currently do not support complete or raw. (complete is available both in PyTorch and NumPy and should be considered as future work.) The arguments overwrite_a, calc_q, and tiles_per_proc are no more used; instead I have introduced procs_to_merge which is a hyperparameter related to TS-QR.
skipping the QR-related unit test (i.e. those from test_qr.py and some from test_svdtools.py) on AMD-hardware has been removed since the new PyTorch/ROCm-images support torch.linalg.qr.

Possible future work:

remove tall-skinny assumption for split=0 by combining TS-QR with Gram-Schmidt-like loop
The choice mode=complete is available both in PyTorch and NumPy and therefore might be included in future versions, too.

Due Diligence

General:
- 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

…stabilized GS for QR (split=1)

ghost · 2024-01-22T14:27:33Z

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

Legend

…chmidt_for_QR

github-actions · 2024-01-22T14:38:27Z

Thank you for the PR!

github-actions · 2024-01-22T14:40:07Z

Thank you for the PR!

mrfh92 · 2024-01-22T15:02:17Z

To be done:

allow batches of matrices instead of matrices only
implement the case split=0 (TS-QR)
implement the case overwrite_a = True
API-details need to be discussed: Change from calc_r, full_q etc. to the NumPy-like mode=‘reduced’, ‘complete’, or ‘r’? --- since we might need the full options lateron, keeping calc_r, full_q in a _qr routine in the background while defining a qr-routine with NumPy-like API in the foreground might be an option

codecov · 2024-01-22T15:06:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.58%. Comparing base (86fe8a6) to head (830dbea).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1329      +/-   ##
==========================================
- Coverage   92.05%   91.58%   -0.48%     
==========================================
  Files          80       80              
  Lines       11950    11647     -303     
==========================================
- Hits        11001    10667     -334     
- Misses        949      980      +31

Flag	Coverage Δ
unit	`91.58% <100.00%> (-0.48%)`	⬇️

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 · 2024-01-26T13:02:30Z

Thank you for the PR!

…chmidt_for_QR

github-actions · 2024-01-26T16:10:04Z

Thank you for the PR!

github-actions · 2024-01-26T16:11:38Z

Thank you for the PR!

…QR' of github.com:helmholtz-analytics/heat into features/1237-Try_out_pure_stabilized_Gram-Schmidt_for_QR

github-actions · 2024-02-27T15:06:57Z

Thank you for the PR!

github-actions · 2024-02-27T15:09:51Z

Thank you for the PR!

…chmidt_for_QR

github-actions · 2024-02-28T14:40:38Z

Thank you for the PR!

…chmidt_for_QR

github-actions · 2024-03-01T08:38:50Z

Thank you for the PR!

…chmidt_for_QR

github-actions · 2024-04-11T09:00:55Z

Thank you for the PR!

…chmidt_for_QR

github-actions · 2024-04-12T14:21:04Z

Thank you for the PR!

github-actions · 2024-04-15T08:52:48Z

Thank you for the PR!

…chmidt_for_QR

github-actions · 2024-04-16T10:14:35Z

Thank you for the PR!

ClaudiaComito

Thanks for all the work, a few comments:

benchmarks/cb/linalg.py

heat/core/linalg/qr.py

ClaudiaComito · 2024-04-16T11:58:52Z

heat/core/linalg/qr.py

+        # handle the case of a single process or split=None: just PyTorch QR
+        Q, R = torch.linalg.qr(A.larray, mode=mode)
+        R = DNDarray(


I disagree, it's not inconsistent to say: we enable all modes for non-distributed DNDarrays, and "reduced" only for distributed DNDarrays.

mrfh92 · 2024-04-17T11:26:13Z

I have addressed all except for the last comment; the last one I didnt understand because I dont see the inconsistency that is not there (?).

github-actions · 2024-04-17T11:29:26Z

Thank you for the PR!

…chmidt_for_QR

ClaudiaComito

Awesome @mrfh92 thank you so much!

github-actions · 2024-04-17T13:58:25Z

Thank you for the PR!

github-actions · 2024-04-17T13:58:30Z

Thank you for the PR!

Hoppe and others added 5 commits October 10, 2023 11:10

added file myqr.py that contains experiment in direction blockwise …

f85a9f2

…stabilized GS for QR (split=1)

Update __init__.py

497b151

removed myrandn since randn is fixed meanwhile

c8f3016

...

134187f

replaced previour QR implementation by the new one

9885afa

mrfh92 and others added 3 commits January 22, 2024 15:28

Merge branch 'main' into features/1237-Try_out_pure_stabilized_Gram-S…

c47f18c

…chmidt_for_QR

...

0b6f87e

refactoring

eb84b53

Hoppe added 2 commits January 26, 2024 12:04

...

f2ae33d

added some unit tests for the refactored qr with split=1

7bf93e3

Hoppe and others added 3 commits January 26, 2024 16:36

...

15d15b0

resolved some bugs related to calc_q, calc_r etc.

f6419d0

Merge branch 'main' into features/1237-Try_out_pure_stabilized_Gram-S…

1b8d8eb

…chmidt_for_QR

Hoppe added 9 commits January 31, 2024 14:51

removed option overwrite_a

85d7a36

extended comments in the code

cdf1603

...

919af4a

Merge branch 'features/1237-Try_out_pure_stabilized_Gram-Schmidt_for_…

03877ae

…QR' of github.com:helmholtz-analytics/heat into features/1237-Try_out_pure_stabilized_Gram-Schmidt_for_QR

removed strange option full_q (should be addressed somewhere else later)

e93ebd0

formatting of warning

56909fa

adapted API of new qr to follow the one in numpy

72e4b20

first version of split=0 (TS-QR), stil with bugs

6e8fda5

fixed some bugs, some others are stil there...

2e42efa

Merge branch 'main' into features/1237-Try_out_pure_stabilized_Gram-S…

e7f15d2

…chmidt_for_QR

Merge branch 'main' into features/1237-Try_out_pure_stabilized_Gram-S…

c433dd8

…chmidt_for_QR

ClaudiaComito added the linalg label Mar 3, 2024

mtar removed the PR talk label Mar 4, 2024

Merge branch 'main' into features/1237-Try_out_pure_stabilized_Gram-S…

719f805

…chmidt_for_QR

ClaudiaComito added this to the 1.4.0 milestone Apr 12, 2024

Merge branch 'main' into features/1237-Try_out_pure_stabilized_Gram-S…

93fbfec

…chmidt_for_QR

updated docs of refactored QR since no batches of matrices are supported

d93659b

mrfh92 requested a review from ClaudiaComito April 15, 2024 10:04

Merge branch 'main' into features/1237-Try_out_pure_stabilized_Gram-S…

0864422

…chmidt_for_QR

ClaudiaComito requested changes Apr 16, 2024

View reviewed changes

modified docs according to review

5e007d3

mrfh92 requested a review from ClaudiaComito April 17, 2024 12:13

Hoppe and others added 2 commits April 17, 2024 15:49

removed "from time import sleep", completed dead end sentence in docs

f816969

Merge branch 'main' into features/1237-Try_out_pure_stabilized_Gram-S…

830dbea

…chmidt_for_QR

ClaudiaComito approved these changes Apr 17, 2024

View reviewed changes

ClaudiaComito merged commit 67bc254 into main Apr 17, 2024
53 of 55 checks passed

ClaudiaComito deleted the features/1237-Try_out_pure_stabilized_Gram-Schmidt_for_QR branch April 17, 2024 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring of QR: stabilized Gram-Schmidt for split=1 and TS-QR for split=0 #1329

Refactoring of QR: stabilized Gram-Schmidt for split=1 and TS-QR for split=0 #1329

mrfh92 commented Jan 22, 2024 •

edited

Loading

ghost commented Jan 22, 2024 •

edited by ghost

Loading

Legend

github-actions bot commented Jan 22, 2024

github-actions bot commented Jan 22, 2024

mrfh92 commented Jan 22, 2024 •

edited

Loading

codecov bot commented Jan 22, 2024 •

edited

Loading

github-actions bot commented Jan 26, 2024

github-actions bot commented Jan 26, 2024

github-actions bot commented Jan 26, 2024

github-actions bot commented Feb 27, 2024

github-actions bot commented Feb 27, 2024

github-actions bot commented Feb 28, 2024

github-actions bot commented Mar 1, 2024

github-actions bot commented Apr 11, 2024

github-actions bot commented Apr 12, 2024

github-actions bot commented Apr 15, 2024

github-actions bot commented Apr 16, 2024

ClaudiaComito left a comment

ClaudiaComito Apr 16, 2024

mrfh92 commented Apr 17, 2024

github-actions bot commented Apr 17, 2024

ClaudiaComito left a comment

github-actions bot commented Apr 17, 2024

github-actions bot commented Apr 17, 2024

Refactoring of QR: stabilized Gram-Schmidt for split=1 and TS-QR for split=0 #1329

Refactoring of QR: stabilized Gram-Schmidt for split=1 and TS-QR for split=0 #1329

Conversation

mrfh92 commented Jan 22, 2024 • edited Loading

Description

Changes in the implementation of QR:

Possible future work:

Due Diligence

ghost commented Jan 22, 2024 • edited by ghost Loading

Legend

github-actions bot commented Jan 22, 2024

github-actions bot commented Jan 22, 2024

mrfh92 commented Jan 22, 2024 • edited Loading

codecov bot commented Jan 22, 2024 • edited Loading

Codecov Report

github-actions bot commented Jan 26, 2024

github-actions bot commented Jan 26, 2024

github-actions bot commented Jan 26, 2024

github-actions bot commented Feb 27, 2024

github-actions bot commented Feb 27, 2024

github-actions bot commented Feb 28, 2024

github-actions bot commented Mar 1, 2024

github-actions bot commented Apr 11, 2024

github-actions bot commented Apr 12, 2024

github-actions bot commented Apr 15, 2024

github-actions bot commented Apr 16, 2024

ClaudiaComito left a comment

Choose a reason for hiding this comment

ClaudiaComito Apr 16, 2024

Choose a reason for hiding this comment

mrfh92 commented Apr 17, 2024

github-actions bot commented Apr 17, 2024

ClaudiaComito left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 17, 2024

github-actions bot commented Apr 17, 2024

mrfh92 commented Jan 22, 2024 •

edited

Loading

ghost commented Jan 22, 2024 •

edited by ghost

Loading

mrfh92 commented Jan 22, 2024 •

edited

Loading

codecov bot commented Jan 22, 2024 •

edited

Loading