ivy icon indicating copy to clipboard operation
ivy copied to clipboard

Diag

Open JamieLine opened this issue 2 years ago • 5 comments

Fixes #4137 and #4138

JamieLine avatar Sep 06 '22 20:09 JamieLine

I think I've made all of the requested changes including the ones mentioned in the other PRs, and made a few other slight improvements. I've checked through it multiple times and I can't see any errors but I may well have simply missed something.

At the time of commit, there's an issue with the tests that is inherited from a recent commit to master, it isn't caused by files in this commit and it appears that the issue is being worked on.

JamieLine avatar Sep 09 '22 20:09 JamieLine

Thanks for having another look, not adding a given to the test for diagflat really was a bit dim-witted, I think that might have been me missing something from a merge or something getting lost, I've added that in now, and I've removed the uints from the unsupported dtypes, but it appears that the tests are ignoring the unsupported dtype flag here anyway, as they keep drawing float16, which causes a crash when running the real Torch function. I've checked through the test in the debugger and that does appear to be whats happening, I've had a quick chat with some people in the internal chat, they've said to either specify the unsupported dtype in the backend (which I cannot do because one or more of these functions doesn't exist in the backend), or add an assume line to the frontend test. To avoid using ivy_np or ivy_torch as you suggested in another review, I've added this assume line as I cannot find an argument to specify to the test itself to avoid a particular type.

I'll get working on the other PRs later today.

JamieLine avatar Sep 12 '22 18:09 JamieLine

Once again GitHub has interesting ideas about when the PR should be closed and what the word merged means.

JamieLine avatar Sep 13 '22 23:09 JamieLine

I've fixed the test in the typical case, turns out there were issues caused by Jax and TF not supporting inplace updates, but the test is currently failing in the real Torch function with the following error which I'll have a look at tomorrow.

2022-09-14T02:37:54.3771751Z >           frontend_ret = frontend_fw.__dict__[fn_tree](*args_frontend, **kwargs_frontend)
2022-09-14T02:37:54.3772012Z E           RuntimeError: [enforce fail at alloc_cpu.cpp:55] ((ptrdiff_t)nbytes) >= 0. alloc_cpu() seems to have been called with negative number: 18446744073709551615 

JamieLine avatar Sep 14 '22 02:09 JamieLine

I'll have a think about if there's a way to do it using ivy functions, but I expect it'll end with either an extension suggestion or some extremely strange bit of linear algebra that's gonna take some thinking to get to. In the meantime I can try to change the implementation to do an inplace update where possible. I've pushed a fix for the Torch crashing, its due to this bug / quirk of PyTorch. https://github.com/pytorch/pytorch/issues/71204

image

JamieLine avatar Sep 14 '22 17:09 JamieLine

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 avatar Nov 07 '22 09:11 ivy-seed

This PR has been closed because it has been marked as stale for more than 7 days with no activity.

ivy-seed avatar Nov 14 '22 06:11 ivy-seed

This PR has been closed because it has been marked as stale for more than 7 days with no activity.

ivy-seed avatar Nov 14 '22 07:11 ivy-seed