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

added instance method isfinite to pytorch tensor (pytorch frontend) #22010

Merged
merged 6 commits into from
Aug 28, 2023

Conversation

VeraChristina
Copy link
Contributor

@VeraChristina VeraChristina commented Aug 16, 2023

Hi, I have added the instance method isfinite.
Close #21943
Would you please have a look? Thanks!

@github-actions
Copy link
Contributor

Thanks for contributing to Ivy! 😊👏
Here are some of the important points from our Contributing Guidelines 📝:
1. Feel free to ignore the run_tests (1), run_tests (2), … jobs, and only look at the display_test_results job. 👀 It contains the following two sections:
- Combined Test Results: This shows the results of all the ivy tests that ran on the PR. ✔️
- New Failures Introduced: This lists the tests that are passing on main, but fail on the PR Fork. Please try to make sure that there are no such tests. 💪
2. The lint / Check formatting / check-formatting tests check for the formatting of your code. 📜 If it fails, please check the exact error message in the logs and fix the same. ⚠️🔧
3. Finally, the test-docstrings / run-docstring-tests check for the changes made in docstrings of the functions. This may be skipped, as well. 📚
Happy coding! 🎉👨‍💻

@VeraChristina
Copy link
Contributor Author

VeraChristina commented Aug 16, 2023

I am not able to link the corresponding issue under development. It is: isfinite #21943
UPDATE: solved!

@VeraChristina VeraChristina changed the title added instance method isfinite to pytorch tensor (pytorch frontend) closes #21943 Aug 16, 2023
@ivy-leaves ivy-leaves added the PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist label Aug 16, 2023
@ivy-leaves
Copy link

If you are working on an open task, please edit the PR description to link to the issue you've created.

For more information, please check ToDo List Issues Guide.

Thank you 🤗

@VeraChristina VeraChristina changed the title closes #21943 Close #21943 Aug 16, 2023
@VeraChristina VeraChristina changed the title Close #21943 added instance method isfinite to pytorch tensor (pytorch frontend) Aug 16, 2023
@umairjavaid
Copy link
Contributor

The tests are not passing. Please fix this and request a review

E   ivy.utils.exceptions.IvyBackendException: numpy: nested_map: numpy: nested_map: can't convert np.ndarray of type bfloat16. The only supported types are: float64, float32, float16, complex64, complex128, int64, int32, int16, int8, uint8, and bool.
E   Falsifying example: test_torch_tensor_isfinite(
E       frontend='torch',
E       on_device='cpu',
E       backend_fw='torch',
E       dtype_and_x=(['bfloat16'], [array(-1, dtype=bfloat16)]),
E       init_flags=FrontendMethodTestFlags(
E           num_positional_args=0,
E           as_variable=[False],
E           native_arrays=[False],
E       ),
E       method_flags=FrontendMethodTestFlags(
E           num_positional_args=0,
E           as_variable=[False],
E           native_arrays=[False],
E       ),
E       frontend_method_data=FrontendMethodData(ivy_init_module='ivy.functional.frontends.torch', framework_init_module='torch', init_name='tensor', method_name='isfinite'),
E   )
E   
E   You can reproduce this example by temporarily adding @reproduce_failure('6.82.4', b'AXicY2YAAkYGCIDRDAAAcgAG') as a decorator on your test case

@VeraChristina
Copy link
Contributor Author

The test test_torch_tensor_isfinite now passes (when run with pytest in my environment), there remain unsuccessful checks, but the error messages are not very telling. Where should I look instead?

@VeraChristina
Copy link
Contributor Author

@umairjavaid Can't request a review in the sidebar -- it just says 'No reviews'. Can you please help with the above question?

@with_unsupported_dtypes(
{
"2.0.1 and below": (
"bfloat16",
Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked isfinite supports bfloat16

{
"2.0.1 and below": (
"bfloat16",
"bool",
Copy link
Contributor

Choose a reason for hiding this comment

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

bool is also supported

"2.0.1 and below": (
"bfloat16",
"bool",
"uint8",
Copy link
Contributor

Choose a reason for hiding this comment

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

uint8 is also supported

"bfloat16",
"bool",
"uint8",
"int8",
Copy link
Contributor

Choose a reason for hiding this comment

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

int8 is also supported

"bool",
"uint8",
"int8",
"int16",
Copy link
Contributor

Choose a reason for hiding this comment

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

int16 is also supported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But all those tests fail at a deeper level than this instance method. What's the reason for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that they are not supported in certain backends? How do I change that?

"uint8",
"int8",
"int16",
"complex64",
Copy link
Contributor

Choose a reason for hiding this comment

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

complex64 is also supported

"int8",
"int16",
"complex64",
"complex128",
Copy link
Contributor

Choose a reason for hiding this comment

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

complex128 is also supported

@VeraChristina
Copy link
Contributor Author

VeraChristina commented Aug 16, 2023

But all those tests fail at a deeper level than this instance method (if I exclude the dtype ommissions). What's the reason for that?
It seems that they are not supported in certain backends? How do I change that?
Compare for instance the test result you copied in your first comment above for bfloat16.

Copy link
Contributor

@umairjavaid umairjavaid left a comment

Choose a reason for hiding this comment

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

Please remove the supported dtype from the @with_unsupported_dtypes

@VeraChristina
Copy link
Contributor Author

VeraChristina commented Aug 16, 2023

I can do that, but then the tests will fail. (See my comment above this one.)

@VeraChristina
Copy link
Contributor Author

I have removed the supported dtypes as requested. Now the tests fail.
I have also run the test test_torch_isfinite for the respective function isfinite in the torch frontend (i.e. the one that my instance method calls) and that test fails (with the same error messages as my test does), so apparently this function does not support these dtypes. Or am I getting this totally wrong? I'd be grateful for feedback.

@VeraChristina
Copy link
Contributor Author

Just for comparison, I ran the tests for the very similar instance method isinf -- same test failures. Please let me know what to do about this PR.

@umairjavaid
Copy link
Contributor

umairjavaid commented Aug 17, 2023

You need add bool to the unsupported dtype decorator of the isfinite tensorflow backend function, so that bool is not passed to it during testing. Similarly, you also need to implement isfinite in paddle backend if it's missing along with its unsupported dtype decorator.

@VeraChristina
Copy link
Contributor Author

VeraChristina commented Aug 17, 2023

Thank you! This was very helpful. I have added bool to unsupp dtypes in tensorflow backend. The function isfinite in the paddle backend is already implemented along with the unsupported dtype decorator (which looks correct to me) and I don't know why it's not found during test. Similarly, I don't know why the test for the torch backend fails; as far as I can see bfloat16 should be supported.

@umairjavaid umairjavaid merged commit 645ec20 into ivy-llc:main Aug 28, 2023
116 of 133 checks passed
umairjavaid added a commit that referenced this pull request Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

isfinite
3 participants