-
Notifications
You must be signed in to change notification settings - Fork 199
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
Move DelayWrapper logic to Proxy #1087
base: master
Are you sure you want to change the base?
Conversation
Related to Xilinx#1023 Move `DelayWrapper` logic to Proxy classes. * Add `DelayWrapper` instantiation in the `WeightQuantProxyFromInjectorBase` class in `src/brevitas/proxy/parameter_quant.py`. * Modify the `forward` method in `WeightQuantProxyFromInjectorBase` to use `DelayWrapper` to decide the return value. * Remove `DelayWrapper` instantiation and usage from the `IntQuant` and `DecoupledIntQuant` classes in `src/brevitas/core/quant/int_base.py`. * Add tests in `tests/brevitas/proxy/test_proxy.py` to ensure the new behavior of `DelayWrapper` in the proxy classes.
Hi @vishwamartur, Thanks for looking at this. I see you've opened two PRs with the same title - which one should I be looking at? This or #1085? Can you close the one that is redundant? Can you also set the merge target for all your PRs (this, #1085, #1086) to Cheers, |
Sure |
Hello, @vishwamartur, Adding to what Nick said, I'd recommend also checking our contribution guidelines, that could help with the submission process of a new PR. In general, it is always appreciated reaching out through an existing or new issue to discuss what problem you would like to solve and discuss at high level your solution, just to make sure that all the relevant part of the codebase are covered and that we could guide you through potential side effects. In this particular case, without taking a closer look at the PR, I am noticing that some weight proxies are missing the update to use delay_wrapper and all the runtime proxies are missing it too (but I see that some of this changes are present in the redudant PR, so let us know which one is the duplicate). Thanks again for your help. |
Hi @nickfraser and @Giuseppe5, Thank you both for the feedback and guidance. I’ll go ahead and close the redundant PR (#1085) and set the merge target to I’ll also review the contribution guidelines and make sure to reach out in the issues section for any future changes to discuss the problem and solution approach at a high level. For this PR, I’ll make the adjustments to ensure that all weight and runtime proxies utilize Thanks again for your support — I appreciate the detailed feedback! |
Hi. Are you still working on this? |
Related to #1023
Move
DelayWrapper
logic to Proxy classes.DelayWrapper
instantiation in theWeightQuantProxyFromInjectorBase
class insrc/brevitas/proxy/parameter_quant.py
.forward
method inWeightQuantProxyFromInjectorBase
to useDelayWrapper
to decide the return value.DelayWrapper
instantiation and usage from theIntQuant
andDecoupledIntQuant
classes insrc/brevitas/core/quant/int_base.py
.tests/brevitas/proxy/test_proxy.py
to ensure the new behavior ofDelayWrapper
in the proxy classes.