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

feat: corrcoef instance method for pytorch frontend and test function added #27032

Closed

Conversation

sharjeelnawaz8182
Copy link
Contributor

@sharjeelnawaz8182 sharjeelnawaz8182 commented Oct 16, 2023

PR Description

Related Issue

Closes #26843

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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

PR Compliance Checks Passed!

@github-actions
Copy link
Contributor

Thank you for this PR, here is the CI results:


This pull request does not result in any additional test failures. Congratulations!

@ivy-leaves ivy-leaves added the PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist label Oct 16, 2023
@hmahmood24 hmahmood24 changed the title corrcoef instance method for pytorch frontend and test function added feat: corrcoef instance method for pytorch frontend and test function added Oct 16, 2023
Copy link
Contributor

@hmahmood24 hmahmood24 left a comment

Choose a reason for hiding this comment

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

Hey @sharjeelnawaz8182, thanks a lot for the PR. However, we need to fix the tests first in order to merge the PR. Please run your tests locally using pytest first to see if they're passing or not. I tested them locally and they were failing for me for all the backends.
image

@sharjeelnawaz8182
Copy link
Contributor Author

sharjeelnawaz8182 commented Oct 16, 2023

@hmahmood24
when i keep
init_input_dtypes=input_dtype
it fails .
but when i test with
init_input_dtypes = ["float"]
it pass for all the tests

Copy link
Contributor

@hmahmood24 hmahmood24 left a comment

Choose a reason for hiding this comment

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

Hey @sharjeelnawaz8182, thanks for the PR. Requesting minor changes. Let me know if the tests still fail after this. Also, it would be helpful if you can share the logs of the test failures here so we can take a look at fixing them. Thanks!

Comment on lines +5589 to +5590
min_value=1,
shared_dtype=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to keep these arguments. You can remove these

init_tree="torch.tensor",
method_name="corrcoef",
dtype_and_x=helpers.dtype_and_values(
available_dtypes=helpers.get_dtypes("float"),
Copy link
Contributor

@hmahmood24 hmahmood24 Oct 26, 2023

Choose a reason for hiding this comment

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

We prefer using "valid" instead of "float" here. Of course you'll then have to update the @with_unsupported_dtypes decorator to correctly flag all the invalid dtypes or inversely use the @with_supported_dtypes to just define the valid dtypes

init_all_as_kwargs_np={
"data": x[0],
},
method_input_dtypes=[],
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to pass input_dtype over here as well

@hmahmood24
Copy link
Contributor

Closing this PR since it has been stale for more than 2 weeks now.

@hmahmood24 hmahmood24 closed this Nov 8, 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.

corrcoef
3 participants