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/functional): update diagflat docstring #23342

Closed
wants to merge 51 commits into from

Conversation

shephinphilip
Copy link
Contributor

@shephinphilip shephinphilip commented Sep 9, 2023

Description

In this updated docstring:

  • The parameter descriptions have been corrected and updated to match the actual parameters of the function.
  • The default values for optional parameters are specified.
  • Examples are provided to demonstrate the usage of the function with both ivy.Array inputs and different values for the offset parameter.

Related Issue

Close #17543

Checklist

  • Updated the docstring for ivy.diagflat() to accurately reflect the function's parameters and behavior.
  • Added tests (if applicable).
  • Followed the steps provided.

Socials:

N/A

shephinphilip and others added 30 commits July 9, 2023 16:45
:func:`ivy.inplace_update` is a *primary* function, and the backend-specific implementations for each backend are presented below.
We also explain the rationale for why each implementation is the way it is, and the important differences.
We also explain the rational for why each implementation is the way it is, and the important differences.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is wrong, similarly in many other places. Please fix/remove these erroneous grammatical changes.

@@ -460,15 +464,15 @@ More information about these arrays can be found in `NumPy's documentation <http
This essentially means that any inplace update on the original array or any of its views will cause all the other views to be updated as well since they reference the same memory buffer.

We want to keep supporting NumPy and PyTorch inplace updates whenever we can and superset backend behaviour, however it is not trivial to replicate this in JAX and TensorFlow.
The main reason is because these frameworks do not natively support inplace updates so even if multiple native arrays are referencing the same memory buffer, you would never be able to update it once for all of them.
The main reason is because these frameworks do not natively support inplace updates so even if multiple native arrays are referencing the same memory buffer, you would ever be able to update it once for all of them.
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, this seems to a wrong fix. There are many such cases in the PR.

Copy link
Contributor

@vaithak vaithak left a comment

Choose a reason for hiding this comment

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

A lot of these changes are wrong and simply not required, please fix them.

@shephinphilip
Copy link
Contributor Author

shephinphilip commented Sep 13, 2023

A lot of these changes are wrong and simply not required, please fix them.

Hi @vaithak, I actually have no idea. All I changed was one file, and after creating a pull request, some of the files are getting changed. All I changed was update diagflat docstring in ivy\functional\ivy\experimental\linear_algebra.py file.

Yeah now it is solved i guess

@ivy-seed ivy-seed assigned YushaArif99 and unassigned vaithak Sep 16, 2023
@shephinphilip
Copy link
Contributor Author

@YushaArif99
It's been a week , since I opened this pull request . Could you please review my PR. I would really appreciate it.

Copy link
Contributor

@YushaArif99 YushaArif99 left a comment

Choose a reason for hiding this comment

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

Hi @shephinphilip

Apologies for the delay in review. Have been swamped with work recently and so havent had the time to review any PRs 😅

Could you kindly address the abovementioned points?

@ivy-seed
Copy link

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
Copy link

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

@ivy-seed ivy-seed closed this Feb 10, 2024
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 Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mismatching Docstring of function ivy.diagflat()
7 participants