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

Eigen op 391 #463

Merged
merged 1 commit into from
May 7, 2023
Merged

Eigen op 391 #463

merged 1 commit into from
May 7, 2023

Conversation

KostasBitsakos
Copy link
Contributor

The EigenOp complete.

Returns two matrixes as results, one for the Eigen Values and one for the Eigen Vectors.
The res1 matrix for eigenvalues and the res2 matrix for eigenvectors
Column k of the returned matrix res2 is an eigenvector corresponding to eigenvalue number k as returned by eigenvalues().
The eigenvectors are normalized to have (Euclidean) norm equal to one.

Also includes the download and build commands for the cpp Eigen library, using the cmake tool.

Copy link
Collaborator

@corepointer corepointer left a comment

Choose a reason for hiding this comment

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

Thank you @KostasBitsakos for your efforts on the initial version.

I've cleaned it up a little, fixed the dependency installation and rebased on the current state of the main branch (rebasing needs force pushing so you will get conflicts that you can circumvent by either hard resetting your local branch to the state on github or moving your local copy out of the way and then checking out the branch again).

Besides addressing the inline comments from the code, please add the following:

  • There's a isSymmetric() function somewhere in the code base (in runtime/local/kernels iirc) that you can use to check for symmetric inputs and gracefully fail with an informative message if your EigenCal kernel is used in another way.
  • Add the necessary bits and pieces to use the kernel from DaphneDSL. Ideally with another test case.

Regards, Mark

src/runtime/local/kernels/kernels.json Outdated Show resolved Hide resolved
test/runtime/local/kernels/EigenCalTest.cpp Outdated Show resolved Hide resolved
@corepointer
Copy link
Collaborator

Hi @KostasBitsakos!
I rebased this branch onto the latest changes in main, resolved some conflicts and removed that one extra commit that added temporary files from the test cases. Once the mentioned issues from the review are resolved, we can merge it in.
Regards, Mark

@corepointer
Copy link
Collaborator

There was a minor error on main. I used merging instead of rebasing to pull this here. This should avoid conflicts and can be fixed with the final merge to main anyway.

And please don't worry about the failing checks (I cancelled the last one running because it would fail anyway). They fail because the Eigen library is not in the Docker image that is used for the test compile yet.

@KostasBitsakos
Copy link
Contributor Author

KostasBitsakos commented Feb 20, 2023 via email

@corepointer
Copy link
Collaborator

I meant the GitHub Action test is failing and that can be ignored (what you see if you click on "Checks" in this pull request). Your own tests should work fine. Please share more details and error messages of your failing tests.

@corepointer
Copy link
Collaborator

Any updates on the compilation issues? If it's still blocking you from fixing the remaining issues (see comments from my code review above) you could use the daphneeu/github-action docker container where all the dependencies (including Eigen) are already installed. There you'd run the build script with ./build.sh --no-deps --installPrefix /usr/local to avoid building the dependencies.
Hope that helps :)

This commit adds the implementation of the eigen() builtin function using the [Eigen library](https://eigen.tuxfamily.org/)

Closes daphne-eu#391, Closes daphne-eu#463
@corepointer corepointer merged commit 8a4681c into daphne-eu:main May 7, 2023
@corepointer
Copy link
Collaborator

LGTM - thx for your efforts @KostasBitsakos. I've merged this with minor cleanup. Please add documentation and a script level test case in another commit.

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