-
Notifications
You must be signed in to change notification settings - Fork 89
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
fix: always check '.length' for 'unknown_length' #3332
Conversation
These changes look reasonable to me 👍 |
It's hard to diagnose because of the JAX PyTrees control flow, but at least one of the errors was caused by this change:
(from But this may be an indication that we need to rethink the JAX backend, too. (I'm still worried about that...) |
I tend to agree. Despite being a JAX fan, I think it is more important that awkward-array with typetracer backend works flawlessly. |
Yeah, unfortunately this kind of logic is not easy to pattern match. When I was reworking this handling, I started to make an effort to standardise unknown length handling, but I definitely think there's a long tail on this. Over time, though, we'll get them all :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look reasonable to me, probably some of these additional checks would never be hit in practice, but better be safe than sorry as we hit exactly these kinds of problems several times in the past. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me! Thanks!
@pfackeldey, is this correct? Here are the matches, post-fix.