implement cov #11193
implement cov #11193
Conversation
ivy-leaves
added
Array API
ivy/array/linear_algebra.py
Outdated
| of weights can be used to assign probabilities to observation vectors. | ||
| dtype | ||
| optional variable to set data-type of the result. By default, data-type | ||
| will have at least ``numpy.float64`` precision. |
There was a problem hiding this comment.
float64 would be more accurate as we are in an ivy function and the dtype will not necessarily be translated to numpy.
ivy/array/linear_algebra.py
Outdated
| an array containing the covariance matrix of an input matrix, or the | ||
| covariance matrix of two variables. The returned array must have a | ||
| floating-point data type determined by Type Promotion Rules and must be | ||
| a square matrix of shape (N, N), where N is the number of rows in the |
There was a problem hiding this comment.
I think that "N is the number of variables in the input(s)" would be more accurate. rowvar will determine whether that's rows or columns in practice.
ivy/array/linear_algebra.py
Outdated
| /, | ||
| *, | ||
| rowVar: Optional[bool] = True, | ||
| bias: Optional[bool] = False, |
There was a problem hiding this comment.
The Optional type-hint is supposed to be used only for arguments that can be None.
ivy/container/linear_algebra.py
Outdated
| ddof: int = None, | ||
| fweights: ivy.Array = None, | ||
| aweights: ivy.Array = None, | ||
| dtype: Optional[Union[ivy.Dtype, ivy.NativeDtype]] = None, |
There was a problem hiding this comment.
According to my previous comment, please add Optional wherever it's needed.
There was a problem hiding this comment.
The same goes for all the argument definitions (backends included).
ivy/array/linear_algebra.py
Outdated
| floating-point data type determined by Type Promotion Rules and must be | ||
| a square matrix of shape (N, N), where N is the number of rows in the | ||
| input(s). | ||
| Examples |
There was a problem hiding this comment.
Please apply the same changes to all of the docstrings in other scripts of the function as well.
| fweights=fweights, | ||
| aweights=aweights, | ||
| ) | ||
|
|
There was a problem hiding this comment.
You're not handling out anywhere, and it is not supported by any native framework for this function. I think you should remove it from the definitions.
|
|
||
| w = None | ||
| if fweights is not None: | ||
| fweights = tf.cast(fweights, dtype=tf.float64) |
There was a problem hiding this comment.
Same comment as for the jax backend.
There was a problem hiding this comment.
I'm not sure casting fweights and aweights makes any difference in the results. Is there a specific reason you have added it in all backends?
ivy/functional/ivy/linear_algebra.py
Outdated
| b: ivy.array([ 1., -1.] | ||
| [-1., 1.]) | ||
| } | ||
| """ |
There was a problem hiding this comment.
It would be great if you added some examples using more of the supported arguments, not just x1 and x2.
| # modifiers: rowVar, bias, ddof | ||
| rowVar = draw(st.booleans()) | ||
| bias = draw(st.booleans()) | ||
| ddof = draw(helpers.ints(min_value=0, max_value=1)) |
There was a problem hiding this comment.
I think we should test for ddof values greater than 1 as well.
| large_abs_safety_factor=2, | ||
| safety_factor_scale="log", | ||
| ), | ||
| test_gradients=st.just(False), |
There was a problem hiding this comment.
If you remove the out argument, you should add test_with_out=st.just(False), here.
|
Hey! Just went through the requested changes and I think they should be done now. The one thing I wasn't sure about making changes to were some of the datatype casts, namely in the Jax, Numpy and Tensorflow backends. Jax's default datatype is float32 and I get a datatype warning that it doesn't match with the ground truth backend (Tensorflow and Numpy both give the same complaint I think). Is there another flag that I could use in the test file to prevent the datatype casting? The datatypes for fweights and aweights are currently hardcoded into the dtypes input array. I assume I should make another PR due to the recent force push again to resolve the conflicts above? |
Hi! |
|
Hey, I think I might've accidentally closed this yesterday when trying to re-commit the changes to a new branch. Should I contact someone about issue 6269 (#6269) being closed by ivy-leaves yesterday (I assume automatically) or can these changes be done via this PR? |
No worries, I opened the issue again and re-linked it to the ToDo list. |
|
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. |
|
Hey, just made a small change removing a flag from the test. I had an issue trying to run the tests locally after the previous force push and I started getting an assertion error: "ground truth backend (tensorflow) returned array on device gpu:0 but target backend (torch) returned array on device cpu" regardless of the function I was trying to test, but I just set up the environment with a docker container and it worked as intended. Are there still some changes I should make or is this version fine as the tests look to pass locally at least for me? |
|
Hi @Hokchris, unfortunately there's been another force push in our repository so you'll need to create a new PR. Apologies for the inconvenience. |
#6269
closes #6269
Implemented cov for linear algebra section.