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

feat: added Ivy.unflatten #28079

Merged
merged 50 commits into from
Feb 3, 2024
Merged

Conversation

Kacper-W-Kozdon
Copy link
Contributor

@Kacper-W-Kozdon Kacper-W-Kozdon commented Jan 27, 2024

PR Description

Adds support for unflatten to the methods of data classes (ivy.Container, ivy.Array).

Related Issue

Closes #28054

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

@ivy-leaves ivy-leaves added Ivy API Experimental Run CI for testing API experimental/New feature or request Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards labels Jan 27, 2024
@Kacper-W-Kozdon
Copy link
Contributor Author

Kacper-W-Kozdon commented Jan 28, 2024

Hi @joaozenobio , would you mind taking a look if this is how it's meant to be set up? I also noticed that a similar function (take) has some extra arguments (mode, fill_value) to deal with some special cases of inputs and outputs. But it seems to come from one of the backends or at least from its implementation in ivy.take(), so it's probably not necessary currently in the unflatten() function?

I've also noticed that ivy_tests/array_api_testing/test_array_api/array_api_tests/test_manipulation_functions.py there are not many tests implemented- does this one should have tests for containers and arrays as well or just ivy_tests/test_ivy/test_functional/test_experimental/test_core/test_manipulation.py (it's not yet implemented but it should be nearly identical to the frontend one)?

If I understood the code correctly, ivy.Container has methods which automatically handle the structure of the container and map the function from ivy onto the leaves?

@Kacper-W-Kozdon Kacper-W-Kozdon marked this pull request as ready for review February 2, 2024 19:51
@Kacper-W-Kozdon
Copy link
Contributor Author

Kacper-W-Kozdon commented Feb 2, 2024

Hey, @KareemMAX , it seems that I have some problems with making it work. Pytest throws errors both on the _static_unflatten and unflatten for containers. In the traceback, the error says that there is something wrong with shape kwarg (I think it was written that it was too long?)- I'm rerunning tests to give more detailed description of the issue.

Edit:

All errors got dealt with, the PR is ready for review & merge.

Copy link
Contributor

@joaozenobio joaozenobio left a comment

Choose a reason for hiding this comment

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

Hi @Kacper-W-Kozdon ! Thanks for your new PR! 🚀 The functions look great, I've noted down some of the corrections in the code that should be done. One thing you need to be sure of is if the dim argument is Optional or not. If one of the backends provides the an optional dim argument, all your functions should provide support for an optional dim too so we can establish the superset behavior. Please check this for each backend and pin me whenever you ready 😄

ivy/data_classes/array/experimental/manipulation.py Outdated Show resolved Hide resolved
ivy/data_classes/container/experimental/manipulation.py Outdated Show resolved Hide resolved
@joaozenobio joaozenobio self-assigned this Feb 3, 2024
@Kacper-W-Kozdon
Copy link
Contributor Author

Hi @Kacper-W-Kozdon ! Thanks for your new PR! 🚀 The functions look great, I've noted down some of the corrections in the code that should be done. One thing you need to be sure of is if the dim argument is Optional or not. If one of the backends provides the an optional dim argument, all your functions should provide support for an optional dim too so we can establish the superset behavior. Please check this for each backend and pin me whenever you ready 😄

Ok, got it- I'll go through the code to ensure that dim is correctly defined.

@Kacper-W-Kozdon
Copy link
Contributor Author

Hi @Kacper-W-Kozdon ! Thanks for your new PR! 🚀 The functions look great, I've noted down some of the corrections in the code that should be done. One thing you need to be sure of is if the dim argument is Optional or not. If one of the backends provides the an optional dim argument, all your functions should provide support for an optional dim too so we can establish the superset behavior. Please check this for each backend and pin me whenever you ready 😄

Only paddle and torch have natively implemented unflatten(), in neither of them dim is optional, so I fixed that. Shape is required as well. Since both are as positional/keyword and both are required, I removed default values from dim and shape. I did that for all the backgrounds covered by pytest.

@joaozenobio
Copy link
Contributor

Hi @Kacper-W-Kozdon ! Perfect, all tests are passing and the code looks in good shape 😄 I'll merge your PR now. Thanks for the contribution and keep up with the hard work 🚀

@joaozenobio joaozenobio merged commit 323b33d into ivy-llc:main Feb 3, 2024
258 of 277 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ivy.unflatten
4 participants