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

adding putmask and its test #18972

Closed
wants to merge 10 commits into from

Conversation

ShreyanshBardia
Copy link
Contributor

closes #18359

@ivy-leaves ivy-leaves added the NumPy Frontend Developing the NumPy Frontend, checklist triggered by commenting add_frontend_checklist label Jul 7, 2023
@bipinKrishnan
Copy link
Contributor

bipinKrishnan commented Jul 11, 2023

Thanks for the contribution @ShreyanshBardia 🙂 . Can you fix the failing test for paddle backend?
Also, it would be nice if you install pre-commit and run it before making a commit, so that the lint checks are done correctly before pushing. Thanks

@bipinKrishnan
Copy link
Contributor

bipinKrishnan commented Jul 11, 2023

ivy-gardener
✅ Ivy gardener has formatted your code.
If changes are requested, don't forget to pull your fork.

@ShreyanshBardia
Copy link
Contributor Author

ShreyanshBardia commented Jul 11, 2023

@bipinKrishnan surely will run precommit beforehand. As for the paddle test case, I am not sure if that is being caused by my PR. A similar test case was failing for #18334, but it appears to be unrelated to the PR. Can you confirm if this is the same case, if not I'll make the required changes.

@ShreyanshBardia
Copy link
Contributor Author

Hi @bipinKrishnan can you check this once

return
if values.size != a.size:
values = np_frontend.resize(values, a.shape)
a[mask] = values[mask]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you forgot the return for this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the documentation, the function doesn't return anything rather changes the array in place. Here is the relevant link

@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 Jul 26, 2023
@ShreyanshBardia
Copy link
Contributor Author

Hi @bipinKrishnan, could you please take a look at this.

@bipinKrishnan
Copy link
Contributor

Hey @ShreyanshBardia , can you fix the failing the test for the function you added, currently failing for the paddle backend

@ShreyanshBardia
Copy link
Contributor Author

Hi @bipinKrishnan, as I mentioned earlier I think the failure is unrelated to the PR, a similar test was failing here #18334 (review) which turned out to be unrelated. Would be glad if you could confirm that

@bipinKrishnan
Copy link
Contributor

Ok, let me check this

@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 Aug 20, 2023
@ShreyanshBardia
Copy link
Contributor Author

ShreyanshBardia commented Aug 21, 2023

Hi @bipinKrishnan, can you take a look at the PR now, I have updated the branch and also updates the test function, since putmask doesn't return anything and changed inplace it is better to test it directly using helpers.value_test. All the tests are passing for me locally

@ShreyanshBardia
Copy link
Contributor Author

@bipinKrishnan It seems an old docker image is being used to test and since I updated my branch, a new dependency that was recently added dill is causing the tests to fail

@bipinKrishnan
Copy link
Contributor

bipinKrishnan commented Aug 22, 2023

It would be great if you could configure pre-commit in your system to avoid lint test fails. Thanks!

@bipinKrishnan
Copy link
Contributor

bipinKrishnan commented Aug 22, 2023

ivy-gardener
✅ Ivy gardener has formatted your code.
If changes are requested, don't forget to pull your fork.

@bipinKrishnan
Copy link
Contributor

@bipinKrishnan It seems an old docker image is being used to test and since I updated my branch, a new dependency that was recently added dill is causing the tests to fail

The team is looking into this issue, let's see how the testing goes once this is fixed. Thanks!

@bipinKrishnan
Copy link
Contributor

Hey @ShreyanshBardia , the issue with dill is fixed now, can you rebase on top of main and try again?

@ShreyanshBardia
Copy link
Contributor Author

Hi, I do have pre-commit configured but I guess this might have been caused when I was resolving merge conflicts using web browser. Apologies for that. It looks like tests ran and were successful can you check @bipinKrishnan

@@ -31,6 +31,15 @@ def fill_diagonal(a, val, wrap=False):
a = ivy.reshape(temp, shape)


@to_ivy_arrays_and_back
def putmask(a, mask, values, /):
Copy link
Contributor

Choose a reason for hiding this comment

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

You could remove the / as it doesn't make sense because there are no kwargs after that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh correct, thanks for catching this, I have updated it correctly now @bipinKrishnan

@bipinKrishnan
Copy link
Contributor

bipinKrishnan commented Sep 6, 2023

ivy-gardener
✅ Ivy gardener has formatted your code.
If changes are requested, don't forget to pull your fork.

@ShreyanshBardia
Copy link
Contributor Author

@bipinKrishnan can you take a look

@ShreyanshBardia ShreyanshBardia deleted the numpy-putmask branch September 13, 2023 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NumPy Frontend Developing the NumPy Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

putmask
5 participants