-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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: adding np.ndarray.var to the frontends #22336
Conversation
Thanks for contributing to Ivy! 😊👏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @PushpamJha14 ! Thanks for opening this PR. Just left a few minor comments. Feel free to request another review when you are done with the changes,.
ivy_tests/test_ivy/test_frontends/test_numpy/test_ndarray/test_ndarray.py
Outdated
Show resolved
Hide resolved
}, | ||
method_all_as_kwargs_np={ | ||
"axis": axis, | ||
"dtype": "float64", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we hardcoding this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method_all_as_kwargs_np={
"axis": axis,
}
Is it fine now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should write a strategy for generating values for the dtype
parameter as well and then add the generated value to this.
@handle_frontend_method( | ||
class_tree=CLASS_TREE, | ||
init_tree="numpy.array", | ||
method_name="var", | ||
dtype_x_axis=helpers.dtype_values_axis( | ||
available_dtypes=helpers.get_dtypes("float"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should generate values for all the parameters of the function, dtype
ddof
, keepdims
and where
are missing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keepdims=st.booleans(),
where=np_frontend_helpers.where(),
dtype=x=helpers.get_dtypes(kind="integer")
ddof=0
Is it right?
method_all_as_kwargs_np={ | ||
"axis": axis, | ||
"dtype": "float64", | ||
"out": None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The testing pipeline will take care of testing the out
argument, no need to pass that as None
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method_all_as_kwargs_np={
"axis": axis,
},
Is it fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remove "out":None
}, | ||
method_all_as_kwargs_np={ | ||
"axis": axis, | ||
"dtype": "float64", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should write a strategy for generating values for the dtype
parameter as well and then add the generated value to this.
method_all_as_kwargs_np={ | ||
"axis": axis, | ||
"dtype": "float64", | ||
"out": None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remove "out":None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor comment this time. Would you please resolve the merge conflict and look into the failing test cases? Feel free to request another review whenever you are done 👍
), | ||
keepdims=st.booleans(), | ||
where=np_frontend_helpers.where(), | ||
dtype=helpers.get_dtypes(kind="integer"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should this only be integer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to valid, is it right now?
dtype_x_axis=helpers.dtype_values_axis( | ||
available_dtypes=helpers.get_dtypes("valid"), | ||
valid_axis=True, | ||
), | ||
keepdims=st.booleans(), | ||
where=np_frontend_helpers.where(), | ||
dtype=helpers.get_dtypes(kind="integer"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can take a look at the test of the frontend np.var
- there's a helper function used there _statistical_dtype_values
which you can use to generate the values. It makes sure that the inputs have reasonable bounds and it will also take care of your ddof
parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to use it I have to import it, is it allowed to import new modules?
I am not getting how to remove these errors especially lint error, can your please help me |
ivy-gardener |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @PushpamJha14! Apologies for the delayed response here. ivy-gardener
should be able to fix your linting errors. As mentioned in the previous review, could you please update the test to use _statistical_dtype_values
to generate the values. Ideally the test should look exactly the same as the corresponding frontend function -- the test for numpy frontend var
function is present in /workspaces/ivy/ivy_tests/test_ivy/test_frontends/test_numpy/test_statistics/test_averages_and_variances.py
. Happy to clarify any other doubts :)
There was a problem hiding this 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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @PushpamJha14, left a minor comment. Could you make sure the tests are passing locally after the change? Happy to review once again after you are done. Thanks :)
def var( | ||
self, axis=None, dtype=None, out=None, ddof=0, keepdims=False, *, where=True | ||
): | ||
return np_frontend.var( | ||
self, | ||
axis=axis, | ||
dtype=dtype, | ||
out=out, | ||
ddof=ddof, | ||
keepdims=keepdims, | ||
where=where, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the tests are failing because they are unable to locate this function (https://github.com/unifyai/ivy/actions/runs/6200453935/job/16835349713?pr=22336#step:3:10495). Could you move this function at bit higher up in the file where other numpy nd.array instance methods are implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes @PushpamJha14 ! The tests are failing on the CI but the traceback is empty, I have informed the CI team about this. Have you tried to run the test locally?
ivy_tests/test_ivy/test_frontends/test_numpy/test_ndarray/test_ndarray.py
Outdated
Show resolved
Hide resolved
…_ndarray.py Co-authored-by: Anwaar Khalid <[email protected]>
Hi, @hello-fri-end I am still getting the same error and warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no failing tests on the CI, will merge this after ivy-gardener fixes the linting errors. Thanks for your contribution @PushpamJha14
Co-authored-by: ivy-branch <[email protected]> Co-authored-by: Anwaar Khalid <[email protected]>
closes #21949