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.

Sign up for GitHub

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

Jump to bottom

Add Aesara backend #214

Closed
wants to merge 1 commit into from
Closed

Add Aesara backend #214

wants to merge 1 commit into from

Conversation

rlouf
Copy link

@rlouf rlouf commented Mar 10, 2023 •
edited
Loading

Description

This PR adds an Aesara backend to opt_einsum following the discussion in #213.

Todos

  • Add Aesara backend
  • Add relevant tests
  • Updated dev environment
  • Updated documentation and README

Status

  • Ready to go

@codecov
Copy link

codecov bot commented Mar 10, 2023 •
edited
Loading

Codecov Report

Merging #214 (b1139d0) into master (1a984b7) will decrease coverage by 0.96%.
The diff coverage is 33.33%.

@rlouf rlouf force-pushed the add-aesara-backend branch from 669e704 to b1139d0 Compare March 10, 2023 09:33
dgasmith
dgasmith approved these changes Mar 24, 2023
View reviewed changes
Copy link
Owner

@dgasmith dgasmith left a comment

Choose a reason for hiding this comment

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

Nice, this LGTM!

@jcmgray Do you want to get eyes on the PR as well before merging?

opt_einsum/tests/test_backends.py
assert isinstance(res_got3, aesara.tensor.TensorVariable)


@pytest.mark.skipif(not found_aesara, reason="Aesara not installed.")
Copy link
Owner

Choose a reason for hiding this comment

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

We really need to turn these into a fixture at some point so we're not duplicating data.

opt_einsum/tests/test_paths.py
@@ -135,7 +132,6 @@ def test_explicit_path():


def test_path_optimal():

Copy link
Owner

Choose a reason for hiding this comment

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

Do these new lines come from a black version bump?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe so, noticed this in a few other libraries as well.

@jcmgray
Copy link
Collaborator

jcmgray commented Mar 24, 2023

All looks good to me, looking forward to testing the performance some time!

By the way @rlouf, I have an implementation of pairwise einsum that should be compatible with with aesara here - https://github.com/jcmgray/einsum_bmm, if aesara needed an implementation. (@dgasmith its also faster than numpy.einsum in many cases, might be something worth including here too....)

@rlouf
Copy link
Author

rlouf commented Mar 26, 2023

That looks awesome! We have an issue that's been open for a while to implement einsum, it would be great to have it in the library.

@dgasmith
Copy link
Owner

dgasmith commented Apr 1, 2023

@rlouf Looks like Asera tests are failing.

@dgasmith
Copy link
Owner

@rlouf Gentle ping here.

@dgasmith
Copy link
Owner

@rlouf Another gentle ping here, we will be shipping another release within the next week.

@rlouf
Copy link
Author

rlouf commented May 27, 2024

I am sorry but I am not working on Aesara anymore. I am closing the PR for now.

@rlouf rlouf closed this May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcmgray jcmgray jcmgray left review comments

@dgasmith dgasmith dgasmith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rlouf @jcmgray @dgasmith