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

Householder reduction to Hessenberg form #34

Merged
merged 3 commits into from
Sep 29, 2023
Merged

Householder reduction to Hessenberg form #34

merged 3 commits into from
Sep 29, 2023

Conversation

paolot-gc
Copy link
Contributor

This branch implements reduction to Hessenberg form using Householder reflections. It makes use of the vertices developed for the QR decomposition.

@paolot-gc paolot-gc requested a review from balancap September 27, 2023 15:45
Copy link
Contributor

@balancap balancap left a comment

Choose a reason for hiding this comment

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

Up to you on how you want to do it, but I am happy if you want to merge the initial implementation of Hessenberg, and then work on the for_iloop version in a new PR/branch. Just need a small unit test before merging (like checking vs Numpy).

Personally a big fan of short PR quickly merged :)

tessellate_ipu/linalg/tile_linalg_hessenberg.py Outdated Show resolved Hide resolved
tessellate_ipu/linalg/tile_linalg_hessenberg.py Outdated Show resolved Hide resolved
@balancap
Copy link
Contributor

Btw, for the CI to pass, you'll need to run the pre-commit: https://github.com/graphcore-research/tessellate-ipu/blob/main/docs/development.md#development
Should be mostly formatting + a bit of flake8 checks

@paolot-gc
Copy link
Contributor Author

I made the changes as per the comments above.
In the accuracy test (i.e. comparison against the scipy implementation), I had to lower the precision (decimal=4) compared to what was set for the QR decomposition (decimal=5).

@paolot-gc
Copy link
Contributor Author

Just to document the effects of the commit above:
This execution trace below (before the addition of ipu_data_barrier()) shows that the Q matrix updates (top tiles) occur after the R matrix updates (bottom tiles)
image
After moving the Q matrix update and adding the `ipu_data_barrier()', the updates occur in parallel
image

@balancap balancap self-requested a review September 29, 2023 08:35
Copy link
Contributor

@balancap balancap left a comment

Choose a reason for hiding this comment

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

Thanks @paolot-gc , looking great 👍

@paolot-gc paolot-gc merged commit 40572ba into main Sep 29, 2023
6 checks passed
@paolot-gc paolot-gc deleted the hessenberg branch September 29, 2023 08:39
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.

2 participants