Added function eigvals #8945

hyadav2k · 2022-12-20T16:25:58Z

kurshakuz

Hey! Thank you for your contribution! I have left a couple of comments here to understand the context better. For now implementation is looking good to me, but I will run tests later. Can you please for now answer to my comments and fix the lint errrors? Thanks!

kurshakuz · 2022-12-21T05:59:28Z

ivy/functional/backends/jax/experimental/linear_algebra.py

@@ -102,4 +102,9 @@ def matrix_exp(


 def eig(x: JaxArray, /) -> Tuple[JaxArray]:
-    return jnp.linalg.eig(x)
+    w, v = jnp.linalg.eig(x)
+    return w, v


You don't need to change other functions in this PR

Sure, it was throwing merge conflicts that's why I changed it. I will undo the changes. Thanks for reviewing.

kurshakuz · 2022-12-21T06:00:32Z

ivy/functional/backends/numpy/experimental/linear_algebra.py

@@ -108,3 +108,10 @@ def eig(x: np.ndarray, /) -> Tuple[np.ndarray]:


 eig.support_native_out = False
+
+
+def eivalsg(x: np.ndarray, /) -> np.ndarray:


Can you please let me know what is eivalsg function here? What is the difference from eivals?

It is a typo. Thanks for pointing out. Will fix it.

kurshakuz · 2022-12-21T06:01:48Z

ivy/functional/backends/torch/experimental/linear_algebra.py

+        ret = torch.linalg.eig(x.to(torch.complex128))
+    else:
+        ret = torch.linalg.eig(x)
+    return tuple(ret)


Was the original function implementation throwing errors before your fix? Maybe we can split this PR into two. In the first you implement eigvals and in the other one you fix eig implementation?

Maybe this was old implementation, that is why I did so. I will try to undo the changes and check once, how it goes. Thanks

hyadav2k · 2022-12-21T16:17:29Z

Hey @kurshakuz, I have resolved the lint errors arising due to my changes.
There is one issue I am facing, the eig for container is not working in my local even though the master is up to date with origin.

        >>> x = ivy.array([[1,2], [3,4]])
        >>> c = ivy.Container({'x':{'xx':x}})
        >>> c.eig()

The o/p is

Traceback (most recent call last):
  File "C:\Program Files\JetBrains\PyCharm 2022.3\plugins\python\helpers\pydev\pydevconsole.py", line 364, in runcode
    coro = func()
           ^^^^^^
  File "<input>", line 1, in <module>
  File "C:\Users\harsh\ivy\ivy\container\linear_algebra.py", line 643, in eig
    return self.static_eig(
           ^^^^^^^^^^^^^^^^
TypeError: ContainerWithLinearAlgebraExperimental.static_eig() got an unexpected keyword argument 'out'

eigvals is also facing the same issue.
Kindly look into it. Thanks.

kurshakuz · 2022-12-27T03:38:23Z

@hyadav2k hey! I have also checked this code snippet and it fails due to the same error. I see that your PR does not depend on eig() method, so I will for now suggest to focus on implementing the eigvals(). The only thing I would ask you is to report this issue in the discord if you can.

Can you then show me the output of the test_eigvals test? If it passes all of them, then it is good to go!

hyadav2k · 2023-01-02T05:01:34Z

The only thing I would ask you is to report this issue in the discord if you can.

Sure, I have reported it in discord.

Can you then show me the output of the test_eigvals test? If it passes all of them, then it is good to go!

Sure, I ran pytest test_linalg.py::test_eigvals. It passed all the tests.

========================================================================================== test session starts ==========================================================================================
platform linux -- Python 3.8.10, pytest-7.1.2, pluggy-1.0.0
rootdir: /workspaces/ivy
plugins: metadata-2.0.4, typeguard-2.13.3, json-report-1.5.0, hypothesis-6.55.0
collected 4 items                                                                                                                                                                                       

test_linalg.py ....                                                                                                                                                                               [100%]

=========================================================================================== warnings summary ============================================================================================
../../../../../../../usr/local/lib/python3.8/dist-packages/flatbuffers/compat.py:19
  /usr/local/lib/python3.8/dist-packages/flatbuffers/compat.py:19: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================================ 4 passed, 1 warning in 74.76s (0:01:14) ================================================================================

@kurshakuz, kindly review the latest commit. Thanks.

kurshakuz

Hey! I have tested your implementation locally and it passing the test. The only thing I think you would need to change is return the tf.Tensor as the only input type to the eig method and remove it from the eigvals as it is stated in the TF documentation. Otherwise LGTM

kurshakuz · 2023-01-02T06:25:00Z

ivy/functional/backends/tensorflow/experimental/linear_algebra.py

@@ -59,9 +59,18 @@ def matrix_exp(


 def eig(
-    x: Union[tf.Tensor],
+    x: Union[tf.Tensor, tf.Variable],


I don't think you need to change this line, as eig only expects Tensor as input as stated here: https://www.tensorflow.org/api_docs/python/tf/linalg/eig

Thanks for reviewing, I made the necessary changes.

kurshakuz · 2023-01-02T06:25:25Z

ivy/functional/backends/tensorflow/experimental/linear_algebra.py

+
+
+def eigvals(
+    x: Union[tf.Tensor, tf.Variable],


Similarly here: https://www.tensorflow.org/api_docs/python/tf/linalg/eigvals

kurshakuz

LGTM! Thank you for your contribution! 🙂

Added function eigvals

e73cb2b

ivy-seed assigned kurshakuz Dec 20, 2022

ivy-leaves added 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 Ivy Functional API labels Dec 20, 2022

kurshakuz reviewed Dec 21, 2022

View reviewed changes

hyadav2k added 5 commits December 21, 2022 12:08

fixed lint errors and merge conflicts

9cd7c86

fixed format

1aaa20f

fixing lint errors

2ab7ae2

Merge remote-tracking branch 'origin' into eigvals

ee2271b

resolving lint errors

6d48d9a

hyadav2k added 2 commits December 30, 2022 19:52

Solve Merge conflicts

9b852c1

fixed failing tests

9866cf6

minor docstring change

ebec12e

kurshakuz reviewed Jan 2, 2023

View reviewed changes

removes extra input argument

8b24170

kurshakuz approved these changes Jan 2, 2023

View reviewed changes

kurshakuz merged commit ca18133 into ivy-llc:master Jan 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added function eigvals #8945

Added function eigvals #8945

hyadav2k commented Dec 20, 2022

kurshakuz left a comment

kurshakuz Dec 21, 2022

hyadav2k Dec 21, 2022

kurshakuz Dec 21, 2022

hyadav2k Dec 21, 2022

kurshakuz Dec 21, 2022

hyadav2k Dec 21, 2022

hyadav2k commented Dec 21, 2022

kurshakuz commented Dec 27, 2022

hyadav2k commented Jan 2, 2023

kurshakuz left a comment

kurshakuz Jan 2, 2023

hyadav2k Jan 2, 2023

kurshakuz Jan 2, 2023

kurshakuz left a comment

		@@ -108,3 +108,10 @@ def eig(x: np.ndarray, /) -> Tuple[np.ndarray]:


		eig.support_native_out = False


		def eivalsg(x: np.ndarray, /) -> np.ndarray:

Added function eigvals #8945

Added function eigvals #8945

Conversation

hyadav2k commented Dec 20, 2022

kurshakuz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyadav2k commented Dec 21, 2022

kurshakuz commented Dec 27, 2022

hyadav2k commented Jan 2, 2023

kurshakuz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurshakuz left a comment

Choose a reason for hiding this comment