-
Notifications
You must be signed in to change notification settings - Fork 569
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
Add transform dispatcher to programs #4559
Conversation
Hello. You may have forgotten to update the changelog!
|
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 great! Thanks Romain
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #4559 +/- ##
=======================================
Coverage 99.66% 99.66%
=======================================
Files 376 376
Lines 33047 33059 +12
=======================================
+ Hits 32937 32949 +12
Misses 110 110
☔ View full report in Codecov by Sentry. |
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
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.
the order still seems strange to me, but it's clear enough. Curious about in-place changes as mentioned in comments.
I'm also wondering - you chose dispatcher.add_to_program(program)
instead of program.add_dispatcher(dispatcher)
. The latter seems more intuitive (kinda like list.append(item)
instead of item.add_to_list(list)
), could you explain why? I almost feel like we can update push_back
to just check if it's passed a dispatcher or a container, then do the right thing depending on that. Or just put it into a new function like push_transform
or push_dispatcher
(the former will make more sense to end-users)
tests/transforms/test_experimental/test_transform_dispatcher.py
Outdated
Show resolved
Hide resolved
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.
this looks really great! to be clear, expand_transforms don't all need to be at the front of a program, just before their associated (regular) transform, right?
Co-authored-by: Matthew Silverman <matthews@xanadu.ai>
Co-authored-by: Matthew Silverman <matthews@xanadu.ai>
Co-authored-by: Matthew Silverman <matthews@xanadu.ai>
@timmysilv the only constrain for expand transforms is that they need to be just before applying the transform itself. |
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.
🎉
Description of the Change:
It makes very easy for users to create a custom
TranformProgram
.This will be used in the device to create the pre processing program.