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

Move DelayWrapper logic to Proxy #1087

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vishwamartur
Copy link

Related to #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.

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.
@nickfraser
Copy link
Collaborator

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 dev, not master?

Cheers,
Nick

@vishwamartur
Copy link
Author

Sure

@Giuseppe5
Copy link
Collaborator

Hello, @vishwamartur,
Thanks for looking into this.

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.

@vishwamartur
Copy link
Author

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 dev for this one as well as #1086.

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 DelayWrapper, as you noted.

Thanks again for your support — I appreciate the detailed feedback!

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