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

frontends.torch.avg_pool2d: initial commit #8854

Merged
merged 4 commits into from
Jan 19, 2023
Merged

frontends.torch.avg_pool2d: initial commit #8854

merged 4 commits into from
Jan 19, 2023

Conversation

hmahmood24
Copy link
Contributor

Close #7790

Initiating this PR to prevent the attached issue from getting stale. The frontend tests for frontends.functional.torch.functional.nn.avg_pool2d are failing right now. However, I'll be working on resolving them within the next few days to get this PR merged as soon as possible. Apologies for any inconvenience.

@ivy-leaves ivy-leaves added the PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist label Dec 18, 2022
@zaeemansari70
Copy link
Contributor

Hey, Sure!
Do request a review from me when needed 🙂

@ivy-seed
Copy link

This PR has been labelled as stale because it has been inactive for more than 7 days. If you would like to continue working on this PR, then please add another comment or this PR will be closed in 7 days.

@ivy-seed ivy-seed added the Stale label Dec 28, 2022
@hmahmood24
Copy link
Contributor Author

Hi @zaeemansari70,

Apologies for the delay. I have spent a decent amount of time on implementing this function and I have multiple concerns and road blocks I'd like you to look into:

  1. First of all, the helper func arrays_for_pooling in ivy/ivy_tests/test_ivy/helpers/hypothesis_helpers/array_helpers.py returns padding argument as a string i.e. sampled from ["VALID", "SAME"], whereas the torch ground truth func torch.nn.functional.avg_pool2d takes padding as ONLY an integer or a tuple of integers (padH, padW) as quoted here. Hence I had to work around that by using ivy.handle_padding(...) within the body of my test func in order to make the test run with the ground truth func. However, ivy.avg_pool2d and all of the respective backend implementations expect padding as a string and so I had to reconvert the integer or tuple input back into a string i.e. one of ["VALID", "SAME"] within my frontend func body. I just don't understand why am I forced to convert to-and-fro one format to another in order to make things work.

  2. Secondly arrays_for_pooling also returns the input tensor in the format of NHWC, whereas the ground truth func requires input format to be either NCW or NCHW as quoted here. Hence I also had to work around that by reshaping the input tensor within the body of my test func.

  3. Thirdly, I don't understand the logic behind ivy.handle_padding as to why isn't padding[i] = (filter[i]-1)/2 used when padding is "SAME" even though that is the formula used to calculate SAME padding size everywhere: image

  4. Fourthly, I don't understand the logic behind the call to torch.nn.functional.pad(...) in ivy/functional/backends/torch/experimental/layers.py::avg_pool2d before calling the ground truth func torch.nn.functional.avg_pool2d(...)? image
    Why explicitly pad the input first and the pass that padded input to the ground truth func with a padding=0 argument?

  5. Lastly, the tests fail with the following error:
    image where the output from my implementation comes out to be different from the ground truth output, but let me demonstrate something here which backs point number 4 above:

The ground truth output for the arguments specified in point number 5 comes out to be array([[[[-0.5],[-1.],[-0.5]]]])

Here's a snippet of my implementation with an explicit call to torch.nn.functional.pad(...) and passing padding=0 in the ground truth func call (as done in the backend implementation in ivy): image The output here mismatches with the ground truth result.

Here's a snippet of my implementation without an explicit call to torch.nn.functional.pad(...) (as it should be in my opinion): image The output here matches with the ground truth result.

So, moving forward, what's the best course of action? Can you talk to someone who implemented these helpers and backend functions to resolve some of my queries and we can get towards merging this PR without any failing tests whatsoever? Thank you! 😊

@Ishticode
Copy link
Contributor

Hi @hmahmood24
Could you kindly resync your branch. It seems to have master merged into and shows 64 changed files. I will take a look as soon as you've updated. Thankyou 🙂

@ivy-leaves ivy-leaves added Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards Ivy API Experimental Run CI for testing API experimental/New feature or request NumPy Frontend Developing the NumPy Frontend, checklist triggered by commenting add_frontend_checklist TensorFlow Frontend Developing the TensorFlow Frontend, checklist triggered by commenting add_frontend_checklist Ivy Functional API labels Jan 3, 2023
@hmahmood24
Copy link
Contributor Author

Hi @Ishticode,
I merged the master into my branch in order to merge any conflicts and get my branch in sync with the master before running any tests hence all those extra commits. My branch is already in sync with master, as for the commit history, you can filter the files that contain just my contribution in the Files changed tab by the following paths:

  • ivy/functional/frontends/torch/nn/functional/pooling_functions.py
  • ivy_tests/test_ivy/test_frontends/test_torch/test_pooling_functions.py

The rest of the files don't concern you as far as my contribution is concerned. As for my queries in the comment above, I have specifically mentioned the file paths I have issues with so you can directly navigate to those. I think this PR has been like this since a long time now and we should not waste any more time on performing redundant tasks like resyncing etc and focus on the main problem at hand to get this resolved ASAP. Hope this helps. 🙂

@hmahmood24
Copy link
Contributor Author

@Ishticode Waiting for a response. Thank you. 🙂

@Ishticode Ishticode removed their request for review January 12, 2023 10:41
@zaeemansari70 zaeemansari70 removed TensorFlow Frontend Developing the TensorFlow Frontend, checklist triggered by commenting add_frontend_checklist NumPy Frontend Developing the NumPy Frontend, checklist triggered by commenting add_frontend_checklist Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards labels Jan 19, 2023
Copy link
Contributor

@zaeemansari70 zaeemansari70 left a comment

Choose a reason for hiding this comment

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

Awesome PR, Thanks for the contribution 🎉.
Merging the PR as some updates are taking place to the pooling functions so any failing tests will be fixed eventually.
Thanks for the patience as well!😊
Merging as discussed with @Ishticode.

@zaeemansari70 zaeemansari70 merged commit ef9cdd3 into ivy-llc:master Jan 19, 2023
@hmahmood24 hmahmood24 deleted the hmahmood-7790-avgpool2d branch February 9, 2023 19:51
vedpatwardhan pushed a commit to vedpatwardhan/ivy that referenced this pull request Feb 23, 2023
vedpatwardhan pushed a commit to vedpatwardhan/ivy that referenced this pull request Feb 26, 2023
vedpatwardhan pushed a commit to vedpatwardhan/ivy that referenced this pull request Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ivy API Experimental Run CI for testing API experimental/New feature or request Ivy Functional API PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

torch.nn.functional.avg_pool2d
5 participants