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(frontends): Implement rfft in PaddlePaddle frontend and fix fft for Tensorflow backend #19454

Merged
merged 12 commits into from
Sep 28, 2023

Conversation

AwkNinja
Copy link
Contributor

Close #19383

@ivy-leaves ivy-leaves added the Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist label Jul 15, 2023
@AwkNinja
Copy link
Contributor Author

Still working on fixing the failing tests for tensorflow and torch.

@AwkNinja
Copy link
Contributor Author

@hmahmood24 Can you please have a look at the implementation here. I can't get the tensorflow and torch tests to pass.

@AwkNinja
Copy link
Contributor Author

The torch tests seem to be passing now but still having issues with tensorflow. The main error is that the elements list is empty so it throws the error cannot draw from an empty list. At first I thought it was because of the norm list that I am generating but it doesn't seem like its related to it.

Comment on lines 185 to 200
dtype_input_axis=helpers.dtype_values_axis(
available_dtypes=helpers.get_dtypes("float"),
shape=(2,),
min_axis=-1,
force_int_axis=True,
),
Copy link
Contributor

@hmahmood24 hmahmood24 Aug 13, 2023

Choose a reason for hiding this comment

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

Why can't we use something like this?

dtype_input_axis=helpers.dtype_values_axis(
        available_dtypes=helpers.get_dtypes("valid"),
        min_num_dims=1,
        min_dim_size=2,
        shape=helpers.get_shape(...),
        force_int_axis=True,
        valid_axis=True,
        allow_neg_axis=True,
    )

force_int_axis=True,
),
norm=st.sampled_from(["backward", "ortho", "forward"]),
n=st.integers(min_value=2, max_value=10),
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to test for the default n=None case

…is setup to accommodate valid data types, adjustable axis
@AwkNinja
Copy link
Contributor Author

Hi @hmahmood24 sorry for the late update but please take a look.

Comment on lines 200 to 202
max_num_dims=2,
min_dim_size=2,
max_dim_size=4,),
Copy link
Contributor

Choose a reason for hiding this comment

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

@asad0991 Perhaps you forgot to install/run pre-commit to fix your lint/formatting errors?

@AwkNinja
Copy link
Contributor Author

@hmahmood24 Fixed the lint issues I changed devices forgot to install pre-commit on the new device.

Copy link
Contributor

@hmahmood24 hmahmood24 left a comment

Choose a reason for hiding this comment

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

@AwkNinja I can see a couple of failing tests still associated with this PR

@AwkNinja
Copy link
Contributor Author

@hmahmood24 The tests for tensorflow gives me keyError: "cup" its like the error you get when the version is wrong in the decorator but I used the correct one. For the paddle error it just started to occur just now. Its basically in ivy.dft function I'll take a look at it.

@hmahmood24
Copy link
Contributor

hmahmood24 commented Sep 11, 2023

@hmahmood24 The tests for tensorflow gives me keyError: "cup" its like the error you get when the version is wrong in the decorator but I used the correct one. For the paddle error it just started to occur just now. Its basically in ivy.dft function I'll take a look at it.

Pretty sure you can find a solution to that key error problem with a simple search in discord since that's been brought up numerous times already

@AwkNinja
Copy link
Contributor Author

AwkNinja commented Sep 11, 2023

@hmahmood24 The tests for tensorflow gives me keyError: "cup" its like the error you get when the version is wrong in the decorator but I used the correct one. For the paddle error it just started to occur just now. Its basically in ivy.dft function I'll take a look at it.

Pretty sure you can find a solution to that key error problem with a simple search in discord since that's been brought up numerous times already

I did read on discord changing version to 2.5.1 and below should have fixed it. I'll have a look again might be another solution someone brought up.

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 Passed!

@a0m0rajab a0m0rajab mentioned this pull request Sep 15, 2023
…due to slight value differences, added conversion from `float` to `complex` as `tensorflow` backend for `fft` doesn't support `float`
@AwkNinja AwkNinja changed the title Implement rfft feature and fix: Implement rfft and fix for tensorflow backend for fft Sep 22, 2023
@AwkNinja
Copy link
Contributor Author

AwkNinja commented Sep 22, 2023

Hi @hmahmood24, the tensorflow tests are passing now with the change I made but It should no longer be needed once this feature is implemented. Also for the paddle tests I found the issue here. Basically query_ is a slice object and at some point value for query_ is slice(0, ivy.Array(2), None) hence giving the error TypeError: slice indices must be integers or None or have an __index__ method. Maybe someone with more experience in the handling of paddle framework can look into this.

@hmahmood24
Copy link
Contributor

Hi @hmahmood24, the tensorflow tests are passing now with the change I made but It should no longer be needed once this feature is implemented. Also for the paddle tests I found the issue here. Basically query_ is a slice object and at some point value for query_ is slice(0, ivy.Array(2), None) hence giving the error TypeError: slice indices must be integers or None or have an __index__ method. Maybe someone with more experience in the handling of paddle framework can look into this.

Hey @AwkNinja, thanks a lot for the effort. I'll ask someone from the team to take a look at this issue you brought up with _parse_query. In the meanwhile I think it should be okay to merge the PR because this issue stems from set_item itself and not with your PR. What do you think @AnnaTz ?

@hmahmood24 hmahmood24 changed the title feature and fix: Implement rfft and fix for tensorflow backend for fft feat(frontends): Implement rfft in PaddlePaddle frontend and fix fft for Tensorflow backend Sep 23, 2023
@AnnaTz
Copy link
Contributor

AnnaTz commented Sep 25, 2023

Hi @hmahmood24, the tensorflow tests are passing now with the change I made but It should no longer be needed once this feature is implemented. Also for the paddle tests I found the issue here. Basically query_ is a slice object and at some point value for query_ is slice(0, ivy.Array(2), None) hence giving the error TypeError: slice indices must be integers or None or have an __index__ method. Maybe someone with more experience in the handling of paddle framework can look into this.

Hey @AwkNinja, thanks a lot for the effort. I'll ask someone from the team to take a look at this issue you brought up with _parse_query. In the meanwhile I think it should be okay to merge the PR because this issue stems from set_item itself and not with your PR. What do you think @AnnaTz ?

This wasn't a set_item issue. I have just resolved it with a quick fix in ivy.dft.

@AwkNinja
Copy link
Contributor Author

Hi @hmahmood24 all the test cases are passing now with the fix that @AnnaTz did. Please take a look.

@hmahmood24 hmahmood24 merged commit d05370e into ivy-llc:main Sep 28, 2023
118 of 137 checks passed
@AwkNinja AwkNinja deleted the rfft branch September 28, 2023 12:28
druvdub pushed a commit to druvdub/ivy that referenced this pull request Oct 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rfft
4 participants