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 code for handling hermitian = True for pinv linear operation in pytorch frontend #22797

Closed
wants to merge 9 commits into from

Conversation

Monsurat-Onabajo
Copy link
Contributor

PR Description

this pull request was created to solve this issue : #22793

Related Issue

Close #22793
#22793

Checklist

  • [✅] Did you add a function?
  • [✅] Did you add the tests?
  • [✅] Did you follow the steps we provided?

Socials:

@onabajo3

@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! 🎉👨‍💻

@ivy-leaves ivy-leaves added the PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist label Aug 30, 2023
@Monsurat-Onabajo
Copy link
Contributor Author

@Mr-Niraj-Kulkarni Can you review my pull request please

Comment on lines 196 to 224
# TODO: add testing for hermitian
@handle_frontend_test(
fn_tree="torch.linalg.pinv",
dtype_and_input=_get_dtype_and_matrix(batch=True),
)
def test_torch_pinv(
*,
dtype_and_input,
frontend,
test_flags,
fn_tree,
backend_fw,
on_device,
hermitian
):
input_dtype, x = dtype_and_input
helpers.test_frontend_function(
input_dtypes=input_dtype,
backend_to_test=backend_fw,
frontend=frontend,
test_flags=test_flags,
fn_tree=fn_tree,
on_device=on_device,
input=x[0],
atol=1e-02,
rtol=1e-02,
hermitian=hermitian
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add only parts specific to your implemented function

@Mr-Niraj-Kulkarni
Copy link
Contributor

@Monsurat-Onabajo Hey, I added some comments on the code Please address those Also I see the tests are failing can you please look into that as well? There are lint issues as well please resolve them

@ivy-leaves ivy-leaves added Ivy Functional API Ivy API Experimental Run CI for testing API experimental/New feature or request labels Aug 31, 2023
@Monsurat-Onabajo
Copy link
Contributor Author

@Mr-Niraj-Kulkarni I have worked on the test reviews. I have also run pre-commit tests to solve the linting errors but i noticed that some of the errors that are coming from there are not from my contribution.

fn_tree,
backend_fw,
on_device,
*, dtype_and_input, frontend, test_flags, fn_tree, backend_fw, on_device, hermitian
Copy link
Contributor

Choose a reason for hiding this comment

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

Please draw the hermitian value from random boolean value generator like below:

@handle_frontend_test(
    fn_tree="torch.linalg.pinv",
    dtype_and_input=_get_dtype_and_matrix(batch=True),
    hermitian=st.booleans(),
)
def test_torch_pinv(
    *,
    dtype_and_input,
    frontend,
    test_flags,
    fn_tree,
    backend_fw,
    on_device,
    hermitian,
):
    input_dtype, x = dtype_and_input
    helpers.test_frontend_function(
        input_dtypes=input_dtype,
        backend_to_test=backend_fw,
        frontend=frontend,
        test_flags=test_flags,
        fn_tree=fn_tree,
        on_device=on_device,
        input=x[0],
        atol=1e-02,
        rtol=1e-02,
        hermitian=hermitian
    )

and please fix the test failures that appear after that

@Monsurat-Onabajo
Copy link
Contributor Author

Hi, @Mr-Niraj-Kulkarni ...I was away on a conference and I did not have access to my computer but i am back now. I tried solving the tasks but there is a circular import that is conflicting with the file i am trying to run, I kept on running into this error. "
ImportError: cannot import name 'array_helpers_dtype_info_helper' from partially initialized module 'ivy_tests.test_ivy.helpers.hypothesis_helpers.array_helpers' (most likely due to a circular import) (C:\Users\Onabajo Monsurat\Documents\venv\lib\site-packages\ivy_tests\test_ivy\helpers\hypothesis_helpers\array_helpers.py)"

@Mr-Niraj-Kulkarni
Copy link
Contributor

Hi, @Mr-Niraj-Kulkarni ...I was away on a conference and I did not have access to my computer but i am back now. I tried solving the tasks but there is a circular import that is conflicting with the file i am trying to run, I kept on running into this error. " ImportError: cannot import name 'array_helpers_dtype_info_helper' from partially initialized module 'ivy_tests.test_ivy.helpers.hypothesis_helpers.array_helpers' (most likely due to a circular import) (C:\Users\Onabajo Monsurat\Documents\venv\lib\site-packages\ivy_tests\test_ivy\helpers\hypothesis_helpers\array_helpers.py)"

Can you please try updating your branch from the main this might solve the issue

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

Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.

Conventional Commit PR Title

In order to be considered for merging, the pull request title must match the specification in conventional commits. You can edit the title in order for this check to pass.
Most often, our PR titles are something like one of these:

  • docs: correct typo in README
  • feat: implement dark mode"
  • fix: correct remove button behavior

Linting Errors

  • Found type "null", must be one of "feat","fix","docs","style","refactor","perf","test","build","ci","chore","revert"
  • No subject found

@Monsurat-Onabajo
Copy link
Contributor Author

Monsurat-Onabajo commented Sep 14, 2023

Hi, @Mr-Niraj-Kulkarni ...I was away on a conference and I did not have access to my computer but i am back now. I tried solving the tasks but there is a circular import that is conflicting with the file i am trying to run, I kept on running into this error. " ImportError: cannot import name 'array_helpers_dtype_info_helper' from partially initialized module 'ivy_tests.test_ivy.helpers.hypothesis_helpers.array_helpers' (most likely due to a circular import) (C:\Users\Onabajo Monsurat\Documents\venv\lib\site-packages\ivy_tests\test_ivy\helpers\hypothesis_helpers\array_helpers.py)"

Can you please try updating your branch from the main this might solve the issue

I have tried updating the main with my local clone but i am still getting the same error. When I checked, the error is coming from 'ivy_tests\test_ivy\helpers\multiprocessing.py' where my file is having issue importing array_helpers_dtype_info_helper. I traced the code to ivy_tests\test_ivy\helpers\hypothesis_helpers\array_helpers.py line 1513 and 1525 'array_helpers_dtype_info_helper' is referencing the backend implementation of it and line 1632 where array_helpers_dtype_info_helper is a function on its own and the code is having hard time knowing which one to use, I am stuck because i dont want to change the name of the function so as not to break any code. Can you also test it from your end to see if there is a way around it

ps: When i read about circular imports, I discovered that it has to do with the way the file is arranged in the init.py file
see here https://stackoverflow.com/questions/64807163/importerror-cannot-import-name-from-partially-initialized-module-m

Copy link
Contributor

@Ishticode Ishticode left a comment

Choose a reason for hiding this comment

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

Hi @Monsurat-Onabajo
Thanks for the PR and sorry for the delay on our end. I will take on your PR from now. Could you pls update your test code in accordance with what @zhumakhan had requested above i.e. add in a boolean strategy for hermitian and resolve the conflicts. Free free to request a further review and update after that. Thank you very much :)

@ivy-seed
Copy link

ivy-seed commented Mar 6, 2024

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 ivy-seed added the Stale label Mar 6, 2024
@ivy-seed ivy-seed closed this Mar 13, 2024
@ivy-seed
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ivy API Experimental Run CI for testing API experimental/New feature or request Ivy Functional API PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<Add Linear Algebra operations to PyTorch frontend >
7 participants