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

Update searching.py #1360

Merged
merged 5 commits into from
Jun 13, 2022
Merged

Update searching.py #1360

merged 5 commits into from
Jun 13, 2022

Conversation

johanjino
Copy link
Contributor

Other than the docstring reformatting, output array was not specified in argmin(). This was observed when trying to run the function on python. Therefore, "out" typing has been specified and passed within function call.

@johanjino johanjino mentioned this pull request Jun 8, 2022
@johanjino
Copy link
Contributor Author

resolves #1357

@Darshan-H-E
Copy link
Contributor

Hey @johanjino, please fix the linting errors in order to pass the formatting check. Add exhaustive docstring examples including instance method examples and container examples. Please refer to https://lets-unify.ai/ivy/deep_dive/11_docstring_examples.html.
Thanks!

The requested documentation on containers and instance methods were added. The linting errors were removed. Please let me know if any more changes are required.
@johanjino
Copy link
Contributor Author

@Darshan-H-E

Thank you for the review. The following changes where made:

  • Added ivy.Container and instance method examples
  • Removed blank lines which was a linting error

Please let me know if anything else needs to be changed.

@Darshan-H-E
Copy link
Contributor

Hi @johanjino, ivy.Array instances will always be inplace updated consistently, in some cases it is simply not possible to also inplace update the ivy.NativeArray which ivy.Array wraps, due to design choices made by each backend. So there is one minor fix that needs to be done. Please remove ivy.NativeArray in the typehint for the out argument. Sorry for the trouble.
Thanks!

@johanjino
Copy link
Contributor Author

@Darshan-H-E

Sure. Thank you again. I have made the changes.

@Darshan-H-E
Copy link
Contributor

@johanjino, I'm sorry if there was any confusion in my explanation. What I meant is change this typehint out: Optional[Union[ivy.Array, ivy.NativeArray]] = None to out: Optional[ivy.Array] = None. The example with native array was correct and needn't be removed. Please restore it.
If you have any doubts, feel free to ask.
Thanks!

@johanjino
Copy link
Contributor Author

@Darshan-H-E

I am sorry for the misunderstanding. I have restored the ivy.nativearray example and removed the respective typehint from the function arguments.

@Darshan-H-E
Copy link
Contributor

Please remove the blank line on line 62.

@johanjino
Copy link
Contributor Author

@Darshan-H-E

Removed the blank line 62. Sorry you had to go through these small changes.

@Darshan-H-E
Copy link
Contributor

It's alright @johanjino. Good job!
Thanks!

@Darshan-H-E Darshan-H-E merged commit aeb17f9 into ivy-llc:master Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants