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

Remove index to access descr #528

Merged
merged 19 commits into from
Jul 30, 2024
Merged

Remove index to access descr #528

merged 19 commits into from
Jul 30, 2024

Conversation

nkoskelo
Copy link
Contributor

@nkoskelo nkoskelo commented Jul 23, 2024 •
edited
Loading

Fixes #522

@inducer
Copy link
Owner

inducer commented Jul 23, 2024

Could you look into the conflicts?

@nkoskelo
Copy link
Contributor Author

In the development of this pull request, I noticed that when a user calls EinsumObj.with_tagged_reduction(axis_descr, tag). Axis_descr can be either an EinsumReductionAxis or a str.

from pytools import Tag

class MyTag(Tag): pass

pt.make_placeholder("foo", (10, 6), float).with_tagged_axis(iaxis: int, MyTag()) Does not require the user pass the actual axis object.
pt.sum(pt.ones(22, float).with_tagged_reduction(varname: str, MyTag()) requires that the user pass the variable name and not the direct object.

This suggests to me that we may want to maintain the functionality that the user can call EinsumObj.with_tagged_reduction(axis_descr: str, tag)

@inducer
Copy link
Owner

inducer commented Jul 23, 2024 •
edited
Loading

As @kaushikcfd points out (#522), that functionality wasn't used for much.

@nkoskelo
Make the index_to_access_descriptor a cached property that will autom…
e00a1c6 
…atically build a dictionary for strings to access descriptor. Note that this may not be the exact same string as what the user passed in pt.einsum, but will be equivalent.
@nkoskelo nkoskelo closed this Jul 23, 2024
@nkoskelo nkoskelo force-pushed the remove_index_to_access_descr branch from e00a1c6 to 550920a Compare July 23, 2024 21:37
@nkoskelo
Copy link
Contributor Author

Reopening pull request now that the merge conflicts have been resolved.

@nkoskelo nkoskelo reopened this Jul 23, 2024
inducer
inducer reviewed Jul 23, 2024
View reviewed changes
pytato/array.py Outdated Show resolved Hide resolved
@nkoskelo nkoskelo marked this pull request as draft July 29, 2024 15:35
@nkoskelo nkoskelo changed the title Remove index to access descr Draft: Remove index to access descr Jul 29, 2024
nkoskelo added 4 commits July 29, 2024 10:42
@nkoskelo
Remove duplicated code in generating a einsum string. The numpy code …
e808591 
…generator returned a string which contained spaces after the punctuation. It now no longer does.
@nkoskelo
Numpy code generator seems to be dependent on the spaces after the pu…
c071fe8 
…nctuation. So, I am adding those back in.
@nkoskelo
Deprecate the old function get_einsum_subscript_str, in favor of get_…
8c407f3 
…einsum_specification. Update the code to use the new function.
@nkoskelo
Visualization needed to be updated as well.
f926d96
@nkoskelo nkoskelo changed the title Draft: Remove index to access descr Remove index to access descr Jul 29, 2024
@nkoskelo nkoskelo marked this pull request as ready for review July 29, 2024 19:09
@nkoskelo nkoskelo requested a review from inducer July 29, 2024 20:16
inducer
inducer reviewed Jul 29, 2024
View reviewed changes
pytato/array.py Outdated Show resolved Hide resolved
pytato/target/python/numpy_like.py Outdated Show resolved Hide resolved
pytato/utils.py Outdated Show resolved Hide resolved
pytato/utils.py Outdated Show resolved Hide resolved
pytato/utils.py Show resolved Hide resolved
pytato/utils.py Outdated Show resolved Hide resolved
pytato/utils.py Outdated Show resolved Hide resolved
@inducer
Copy link
Owner

inducer commented Jul 29, 2024

Unsubscribing... @-mention or request review once it's ready for a look or needs attention.

@nkoskelo nkoskelo requested a review from inducer July 30, 2024 15:36
@inducer inducer enabled auto-merge (squash) July 30, 2024 15:47
@inducer
Copy link
Owner

inducer commented Jul 30, 2024

Thx!

@inducer inducer merged commit 0030ec2 into inducer:main Jul 30, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inducer inducer Awaiting requested review from inducer

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

Successfully merging this pull request may close these issues.

Drop Einsum.index_to_access_descr?
2 participants
@nkoskelo @inducer