-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
add numpy spacing frontend function #16436
Conversation
Including comments to provided clarity on the code - radian - arctan2 - hypot
@juliagsy I have made some updates to the code in the trigonometry python file for the NumPy Frontend pull request. The changes include more additional trigonometry functions, and I added comments to provide clarity. Could you please review the updated code and provide feedback? I would appreciate your guidance to ensure the changes meet the required standards. Thank you in advance for your time and assistance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Thanks for the changes! We actually have a list of open tasks here, where you can then find the todo lists in the repo's GitHub issues. You'll then need to select an available function to work on for the frontends category.
As for the docstrings, they actually don't apply here as this is the frontends, and the functions are identical to their native function, therefore self-explanatory through their native documentation and would be redundant to add docstrings here.
As for the functions you've added/implemented, please first make sure that they are available in the todo list (not marked as completed or ticked in the todo), more information in the open tasks section of the documentation, https://unify.ai/docs/ivy/overview/contributing/open_tasks.html
Thanks!
approx_min_k added to lax/operators.py
approx_min_k function added to lax/operators.py
Hi @juliagsy, thank you for your response. I wanted to let you know that I have made some updates to the code. I have removed the docstrings from it. Additionally, before starting to work on the file, I did select a task from the ToDo list, specifically the 'spacing' task, as I noticed that it was still available and hadn't been assigned to anyone else. I hope this clears up any confusion. Thank you in advance. |
@OziomaEunice Thanks for the clarification! Did you have an issue for that? Could you please link it in the PR or comment with the issue? Thanks a lot! |
@juliagsy I just found out there was an error on my part, I chose the 'spacing' task but instead I did radian, arctan2 and hypot :( !
While the ToDo (Sub-task) is:
I've corrected this by adding the spacing function in the floating_point_routines.py file. Also, I've sent another PR. Kindly see the update. Cheers! |
Added Spacing function to floating_point_routines.py file and test case in the test_floating_point_routines.py file
@OziomaEunice Thanks for clarifying! I see you mention sending a new PR, would you mind to either maybe open a new one which consists of only the |
Update: 1. Removed my code from the trigonometric_functions.py file 2. Spacing function now the one included in the floating_point_routines.py file
I made 2 new PR which now contain the 'spacing' function and its test. |
@OziomaEunice Ahh I see, thanks for the clarification! Since both PRs are about the same changes, would you mind to keep only the newer one of them to avoid duplicates? Thanks a lot! |
@juliagsy Ok, I've kept this one here, and removed the other duplicate. |
Do I merge the branch or is it up to you to do it? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Thanks for contributing! I've left some comments below, feel free to ping me if there's any issue, thanks!
@@ -626,3 +626,13 @@ def conj(x): | |||
@to_ivy_arrays_and_back | |||
def is_finite(x): | |||
return ivy.isfinite(x) | |||
|
|||
|
|||
# approx_min_k |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this function as it is not related to the spacing
function
@@ -10,7 +10,8 @@ | |||
handle_numpy_casting, | |||
) | |||
|
|||
|
|||
# This is the nextafter function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment not needed
|
||
|
||
|
||
# This is the spacing function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment not needed
dtype=None, | ||
subok=True, | ||
): | ||
return ivy.spacing(x, out=out) # Return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the spacing function is not yet available in the ivy functional API, please either implement this frontend function through compositions, or in other cases, propose the function to be implemented in the ivy API, more info here: https://unify.ai/docs/ivy/overview/contributing/open_tasks.html#ivy-experimental-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @juliagsy ! In place of
return ivy.spacing(x, out=out)
I replaced it with # Implement the frontend function using Ivy compositions spacing = ivy.subtract(ivy.nextafter(ivy.abs(x), ivy.sign(x)), x) return spacing
.
Is that ok?
@juliagsy Updates made:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Thanks for the changes so far! If the tests are passing, the approach is fine. Another addition required is in the ivy/frontends/numpy/init.py file. Please import the namespaces at the appropriate location.
- around line 631
- around line 713
Could you please update your fork/branch so that it is latest as ivy master as well? Thanks a lot!
@juliagsy I've done the updates as requested. Cheers :) |
@juliagsy are you able to solve the conflicting files? The only thing showing that is conflicting is the addition of spacing function in the init.py file. |
Hey thanks @juliagsy ! Do I commit the branch or will you be able to do it? |
Hey @OziomaEunice ! The imports should be fine now!
I've tested and they should be passing, but feel free to verify them again, please ping me when the related changes are done! Thanks! |
Oh! And one last thing, you'll need to set these in the
|
@juliagsy no worries, I'll do that and push the code in few minutes |
Also, do I need to put this code in a file? And if so, where? |
@OziomaEunice this should be the complete function body for |
Sorry...again just for clarification, do I add it to the _spacing function? Also, which lines do I add this in the test_floating_point_routines.py:
|
@OziomaEunice yep, that's right! to be specific, it should look like this, thanks!:
|
Thank you! I've update the code now. What remains is this: rtol=1e-03, atol=1e-03, But in which lines do I place this in the test_floating_point_routines.py? |
no problem!
|
@juliagsy all the files have been updated. |
@OziomaEunice I'll wait for the final CI test to complete its run, if everything looks good, I'll go ahead and merge it, thanks again! |
@juliagsy No problem, thanks again! |
Thanks for contributing! |
Including comments to provided clarity on the code
closes #16433