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

Copy axes and tags in zeros_like/full_like #229

Merged
merged 1 commit into from
May 5, 2023

Conversation

majosm
Copy link
Collaborator

@majosm majosm commented May 5, 2023

Am I doing this right? (As far as I could tell, pt.zeros/pt.full don't accept axes or tags directly.)

This makes all of the RHS "uninferred axes" warnings go away in my Y3 prediction smoke_test run.

cc @anderson2981

@inducer inducer enabled auto-merge (rebase) May 5, 2023 17:11
@inducer
Copy link
Owner

inducer commented May 5, 2023

LGTM!

@inducer
Copy link
Owner

inducer commented May 5, 2023

(And thanks for getting that done.)

@inducer inducer merged commit 00e008b into inducer:main May 5, 2023
@majosm
Copy link
Collaborator Author

majosm commented May 5, 2023

Looks like this change is making the stock prediction driver (not my slightly-modified version) fail CI: https://github.com/illinois-ceesd/mirgecom/actions/runs/4896641880/jobs/8744004554. @MTCam confirmed by swapping arraycontext versions. Any idea what could be causing this @inducer, @kaushikcfd?

@inducer
Copy link
Owner

inducer commented May 5, 2023

I think I understand how this could happen, and I think one way to obviate the issue for now would be to delete the offending assert.

FEMEinsumTags are supposed to come only into existence as part of the transform path, and so it's a problem if they exist "before they're even supposed to be created". The bit of code that the assert is part of ensures that only "input-type" arrays (Placeholder, DataWrapper) have this issue. This change to zeros_like permits that tag to "jump" to IndexLambdas, too. I suppose that the cleaner solution would ultimately be to ensure that the tag does not escape the transform process, but deleting the assert should do the trick for now.

@kaushikcfd, LMK if I've got this wrong.

@kaushikcfd
Copy link
Collaborator

That assessment seems correct. The FEMEinsumTag was a broken abstraction, the newest version of kaushikcfd@meshmode is based on the implementation in #217 which gets rid of them.

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.

3 participants