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

added unflatten (incomplete) #25975

Closed
wants to merge 5 commits into from
Closed

added unflatten (incomplete) #25975

wants to merge 5 commits into from

Conversation

HaoES
Copy link

@HaoES HaoES commented Sep 23, 2023

PR Description

added unflatten

Related Issue

Can you please check the implementation and guide me on how to write and run tests?

Close #25905

Checklist

  • Did you add a function?
  • Did you add the tests?
  • Did you run your tests and are your tests passing?
  • Did pre-commit not fail on any check?
  • Did you follow the steps we provided?

Socials:

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

PR Compliance Checks

Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.

Conventional Commit PR Title

In order to be considered for merging, the pull request title must match the specification in conventional commits. You can edit the title in order for this check to pass.
Most often, our PR titles are something like one of these:

  • docs: correct typo in README
  • feat: implement dark mode"
  • fix: correct remove button behavior

Linting Errors

  • Found type "null", must be one of "feat","fix","docs","style","refactor","perf","test","build","ci","chore","revert"
  • No subject found

@ivy-leaves ivy-leaves added PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist Ivy Functional API Ivy API Experimental Run CI for testing API experimental/New feature or request labels Sep 23, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first Pull Request and thanks for supporting Ivy! 🎉
Join the conversation in our Discord

Here are some notes to understand our tests:

  • We have merged all the tests in one file called `display_test_results` job. 👀 It contains the following two sections:
    • Combined Test Results: This shows the results of all the ivy tests that ran on the PR. ✔️
    • New Failures Introduced: This lists the tests that fails on this PR.

Please make sure they are passing. 💪

Keep in mind that we will assign an engineer for this task and they will look at it based on the workload that they have, kindly be patient 😄.

@HaoES HaoES marked this pull request as draft September 24, 2023 16:33
@HaoES HaoES changed the title added unflatten (uncomplete) added unflatten (incomplete) Sep 24, 2023
@HaiderSultanArc
Copy link
Contributor

🤔 Seems like you have pushed a wrong commit for docs submodule

@HaoES
Copy link
Author

HaoES commented Sep 25, 2023

@HaiderSultanArc I only modified three files but I see now that the commit modified also demos_conf.py which I did not touch at all. How can I fix this?

@HaiderSultanArc
Copy link
Contributor

HaiderSultanArc commented Sep 25, 2023

I think you can check what commit docs/demos submodule is on main branch. Then go to demos and checkout to that commit and then commit that 🤔

cd docs/demos
git checkout <COMMIT_HASH_ON_MAIN>
cd ../../
git add -A
git commit -m "update: demos submodule"
git push

@HaoES
Copy link
Author

HaoES commented Sep 26, 2023

@HaiderSultanArc I just did that, I hope things are good now.

@HaiderSultanArc
Copy link
Contributor

Yep it's good now. There are some failing tests. I think you need to implement backend implementations as well right? 🤔

@HaoES
Copy link
Author

HaoES commented Sep 26, 2023

@HaiderSultanArc thank you for always replying, I don't think I should implement the backend implementation as well (The task is called: Add Instance methods to Pytorch Frontend) I guess now I should implement the tests, can you please guide on where I can learn how to do that?

@HaiderSultanArc
Copy link
Contributor

@HaoES If you are working on a frontend task and the backend is not implemented then you will have to implement that before the frontend 😄
As you have implemented the functional API as well it means backends are not implemented. So you have to look through all the frameworks that we support torch, tensorflow, numpy, jax, paddle and see which ones do and don't have native unflatten implementation of their own.
That would tell you if you have to implement mixed, primary or compositional implementation of this function.

@HaoES
Copy link
Author

HaoES commented Sep 27, 2023

@HaiderSultanArc the task I am working on is Add Instance Methods to PyTorch Frontend so it's only concerned with Pytorch (the only framework I know). pytorch have already an unflatten method that I used in the implementation, so I guess the backend side for pytorch is already taken care of? As for the other framework unfortunately I am not familiar with them, I just want to implement unflatten for pytorch as the issue says. Can you please check and tell me what are the next steps I need to do (I guess I should write tests no?). Thank you.

@HaiderSultanArc
Copy link
Contributor

Hey @HaoES you can use the torch.unflatten method for torch backend. But for other backends you will need to implement the behaviour of unflatten with ivy functions if those backends don't have an implementation for this method. If let's say tensorflow also has unflatten then you can use that for tensorflow backend 😄

@HaoES
Copy link
Author

HaoES commented Sep 27, 2023

@HaiderSultanArc I am very thankful for your constant feedback, I totally understand you, but what I understood is that my task focuses only on Pytorch Add Instance Methods to PyTorch Frontend. So I am only working on adding unflatten to Pytorch frontend only, adding it to tensorflow,paddle and the rest is out of the scope of this task, please correct me if I am wrong and if this issue is also about implementing the methods to other frameworks as well.

@HaiderSultanArc
Copy link
Contributor

HaiderSultanArc commented Sep 27, 2023

@HaoES PyTorch Frontend works with every backend. You can run PyTorch frontend with TensorFlow backend as well. So backend is a requirement for frontend functions, and all of them need to be implemented before frontend.

@HaoES
Copy link
Author

HaoES commented Sep 28, 2023

@HaiderSultanArc I understand, I don't know how to implement unflatten for other backends unfortunately :/ I will take some time and check Deep Dive and documentation. And see if I can do that. Thank you for your assistance and patience.

Copy link
Contributor

@KareemMAX KareemMAX left a comment

Choose a reason for hiding this comment

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

Hi, seems like you have changed docs/demos which you might have done by mistake. Can you revert that change because we can't merge with that change in place?

@HaoES
Copy link
Author

HaoES commented Sep 28, 2023

Hello @KareemMAX thank you for your review, can you please help me figure out how to do it?
I already did this as pointed out by @HaiderSultanArc

cd docs/demos
git checkout <COMMIT_HASH_ON_MAIN>
cd ../../
git add -A
git commit -m "update: demos submodule"
git push

@HaiderSultanArc
Copy link
Contributor

@HaiderSultanArc I understand, I don't know how to implement unflatten for other backends unfortunately :/ I will take some time and check Deep Dive and documentation. And see if I can do that. Thank you for your assistance and patience.

Sounds good to me 😄. Take your time!

@HaiderSultanArc
Copy link
Contributor

HaiderSultanArc commented Sep 28, 2023

Hello @KareemMAX thank you for your review, can you please help me figure out how to do it? I already did this as pointed out by @HaiderSultanArc

cd docs/demos
git checkout <COMMIT_HASH_ON_MAIN>
cd ../../
git add -A
git commit -m "update: demos submodule"
git push

It's fixed now. Pull the new changes and try these commands again

@handle_out_argument
@to_native_arrays_and_back
@handle_array_function
@handle_device_shifting
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be undefined 🤔

Copy link
Author

Choose a reason for hiding this comment

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

honestly I don't have any idea what these decorators do XD I just copied them from the reshape function because I assumed that unflatten has the same behavior as reshape.

Copy link
Contributor

Choose a reason for hiding this comment

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

You also have to import them. You can copy the import as well from where you picked this up 😄

Copy link
Contributor

@HaiderSultanArc HaiderSultanArc left a comment

Choose a reason for hiding this comment

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

Current demos submodule commit on main branch of ivy is: 7c99cb5

Use the following commands:

git fetch upstream
cd docs/demos
git checkout 7c99cb5
cd ../../
git add -A
git commit -m "update: demos submodule"
git push origin PR

@HaoES HaoES closed this by deleting the head repository Oct 5, 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.

unflatten
5 participants