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

fix: ivy.linalg.matrix_norm() for paddle backend #28500

Merged
merged 130 commits into from
Mar 15, 2024

Conversation

Kacper-W-Kozdon
Copy link
Contributor

@Kacper-W-Kozdon Kacper-W-Kozdon commented Mar 7, 2024

PR Description

  • Type-casting to the dtype argument through the @infer_dtype decorator and ivy.astype(x, dtype).as_native().
  • Added dtype argument to array and container methods.
  • Added type-casting to float32 for the out argument passed down to torch backend- out argument has to be of the same type as the return of torch.linalg.matrix_norm() (float32).
  • Test coverage for all valid dtypes.

Current issues:

  • Gradient tests for paddle have problems with max() and min() functions (I will write just for max()) due to different implementations between backends (look up paddle.max() vs paddle.amax() or torch.max() vs torch.amax()). Tensorflow, numpy, torch and jax pass all the tests alike.

Related Issue

Continuation of the issue #28314 and PR #28323

Closes #28314

Checklist

  • Did you add a function?
  • Did you add the tests?
  • Did you run your tests and are your tests passing?
  • Did pre-commit not fail on any check?
  • Did you follow the steps we provided?

Socials

@Kacper-W-Kozdon
Copy link
Contributor Author

Kacper-W-Kozdon commented Mar 11, 2024

Hey @vedpatwardhan , new observation (which might be a coincidence but might also point to the issue with paddle backend)- whenever ivy.linalg.matrix_norm() for ord in [-float("inf"), -2, -1, 1, 2, float("inf")] returns rubbish gradients when the backend uses amax, it returns the correct results with max; and the other way around is true (and happens) as well- when max returns wrong results, amax returns the correct ones. I have zero clue what is wrong with paddle but it's not the fault of ivy's implementation of gradients or anything else- I have been checking it with raw paddle to compare the results to ivy's ones on the side as a sanity check. My suggestion would be either opening an issue directly in the paddle github or modifying how amax is handled in the ivy backend. The fact that when the gradient of amax returns rubbish results, max works, suggests, that the problem occurs when there are no multiples in the outputs of max.

EDIT:
If you wish, I can prepare a jupyter notebook with an explanation, because it seems that the results might be salvageable. In fact, maybe even without touching gradients, but that would require noticable changes in the paddle backend, either directly in the min()/max() functions or just in the matrix_norm().

@vedpatwardhan
Copy link
Contributor

Hey @vedpatwardhan , new observation (which might be a coincidence but might also point to the issue with paddle backend)- whenever ivy.linalg.matrix_norm() for ord in [-float("inf"), -2, -1, 1, 2, float("inf")] returns rubbish gradients when the backend uses amax, it returns the correct results with max; and the other way around is true (and happens) as well- when max returns wrong results, amax returns the correct ones. I have zero clue what is wrong with paddle but it's not the fault of ivy's implementation of gradients or anything else- I have been checking it with raw paddle to compare the results to ivy's ones on the side as a sanity check. My suggestion would be either opening an issue directly in the paddle github or modifying how amax is handled in the ivy backend. The fact that when the gradient of amax returns rubbish results, max works, suggests, that the problem occurs when there are no multiples in the outputs of max.

EDIT: If you wish, I can prepare a jupyter notebook with an explanation, because it seems that the results might be salvageable. In fact, maybe even without touching gradients, but that would require noticable changes in the paddle backend, either directly in the min()/max() functions or just in the matrix_norm().

I'd say let's create a minimal example of the incorrect gradients and create an issue on the paddle repo regarding this, but probably what we'll get as the reply is that inf is the problem. Thanks for the efforts so far @Kacper-W-Kozdon 😄

@Kacper-W-Kozdon
Copy link
Contributor Author

Kacper-W-Kozdon commented Mar 12, 2024

Hey @vedpatwardhan , I implemented the workaround, it works- with --num-examples=500 all of the tests passed (including gradient tests) 😄 Could you double-check it? Especially in the paddle backend, statistical.py::min and statistical.py::max

Copy link
Contributor

@vedpatwardhan vedpatwardhan left a comment

Choose a reason for hiding this comment

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

Hey @Kacper-W-Kozdon, seems like there's a few other test failures that have arised as a result of these changes (e.g. this), could you please take a look at this and other failures that are pointed in the workflow if any of them are related? Thanks 😄

@Kacper-W-Kozdon
Copy link
Contributor Author

Hey @Kacper-W-Kozdon, seems like there's a few other test failures that have arised as a result of these changes (e.g. this), could you please take a look at this and other failures that are pointed in the workflow if any of them are related? Thanks 😄

Oh! My bad, thanks for checking that thoroughly- the changes to min() and max() caused problems in other functions because I did not consider NoneType input for the axis argument 👍should be fixed now but we'll see what the workflow shows.

Copy link
Contributor

@vedpatwardhan vedpatwardhan left a comment

Choose a reason for hiding this comment

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

lgtm, there don't seem to be any additional failures introduced. Feel free to merge the PR @Ishticode, thanks @Kacper-W-Kozdon 😄

@Ishticode Ishticode merged commit b30fe90 into ivy-llc:main Mar 15, 2024
180 of 277 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards Ivy Functional API Paddle Paddle Backend PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Frontend Failing Test: torch - linalg.torch.linalg.matrix_norm
6 participants