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

Skip interpolation validator if tracing autograd boxes #1736

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

yaugenst-flex
Copy link
Contributor

@yaugenst-flex yaugenst-flex commented Jun 3, 2024

Fixes #1734

Note: Checking for ArrayBox specifically because the validator should still fail in other instances where self.values might have the wrong dtype.

"""
# skip this validator if currently tracing for autograd because
# self.values will be dtype('object') and not interpolatable
if isbox(self.values.ravel()[0]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @yaugenst-flex

My only concern is whether .ravel() will be slow, but I suppose interpolation already in the validator will be a bit slow too. There's also self.values.flat[0], I'm not sure the difference but this might be a little faster as it returns an iterator rather than a new array.

Another option is that I think when there are tracers in the values, then the full traced ArrayBox will be in self.attrs[AUTOGRAD_KEY], so we could check if AUTOGRAD_KEY in self.attrs:, although it's probably less robust.

Copy link
Collaborator

Choose a reason for hiding this comment

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

another option could be to skip if self.values.dtype == object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's also self.values.flat[0], I'm not sure the difference but this might be a little faster as it returns an iterator rather than a new array.

ravel() only returns a copy if necessary (i.e., the array is not contiguous), but yeah .flat[0] is probably better.

another option could be to skip if self.values.dtype == object

wanted to avoid this since I think it's too general, the validator should generally error on dtype=object

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok sounds good to me (re flat vs ravel vs dtype).

@tylerflex tylerflex self-requested a review June 3, 2024 15:29
@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/fix-interp-validator branch from 09de424 to 626520a Compare June 3, 2024 15:32
Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Looks good except latest submodules is failing (which I guess means it is on pre/2.7), not sure if you want to update it together with this PR?

@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/fix-interp-validator branch from 626520a to 666c61a Compare June 3, 2024 16:37
@yaugenst-flex
Copy link
Contributor Author

@momchil-flex fixed. this is ready to merge then i think

@tylerflex tylerflex merged commit fb44160 into pre/2.7 Jun 3, 2024
16 checks passed
@tylerflex tylerflex deleted the yaugenst-flex/fix-interp-validator branch June 3, 2024 16:56
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