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

frexp implementation #11182

Merged
merged 3 commits into from
Mar 1, 2023
Merged

frexp implementation #11182

merged 3 commits into from
Mar 1, 2023

Conversation

pillarxyz
Copy link
Contributor

@ivy-leaves ivy-leaves added Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards Ivy API Experimental Run CI for testing API experimental/New feature or request Ivy Functional API labels Feb 28, 2023
@nassimberrada
Copy link
Contributor

@pillarxyz Looks mostly good, a couple comments:

  • Running the tests locally throws the following error: TypeError: static_frexp() takes 1 positional argument but 2 positional arguments (and 1 keyword-only argument) were given, could you try to solve this ?
  • At the level of numpy's backend, correct me if i'm wrong but I guess you don't need to add a conditional statement for the case where out=None if np.frexp can handle that case directly
    Let me know when you'd like me to give it another review, thanks !

@pillarxyz
Copy link
Contributor Author

there is an error when I dont add the if out is None: numpy: frexp: 'out' must be a tuple of arrays

I think I added that to the type hints but I think I should specify that out must be a tuple in the tests

@pillarxyz
Copy link
Contributor Author

TypeError: static_frexp() takes 1 positional argument but 2 positional arguments (and 1 keyword-only argument) were given

humm, I dont seem to get that error myself when I run the tests

@nassimberrada
Copy link
Contributor

If you take a look at the intelligent tests you can see a falsifying example for the frexp function which shows the following error TypeError: 'out' must be a tuple of arrays which I think is due to the out argument not being tested. Maybe you could try adding with_out=True to the helpers.test_function

@pillarxyz
Copy link
Contributor Author

this is exactly why I added handling for out
image

@pillarxyz
Copy link
Contributor Author

I think I solved it by making (None, None) the default if out is not set

@nassimberrada
Copy link
Contributor

Looks good to go, thanks !

@nassimberrada nassimberrada merged commit e58f8ca into ivy-llc:master Mar 1, 2023
@pillarxyz pillarxyz deleted the frexp branch March 1, 2023 18:28
vedpatwardhan pushed a commit that referenced this pull request Mar 6, 2023
Co-author: pillarxyz <[email protected]>, nassimberrada <Nassim>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards Ivy API Experimental Run CI for testing API experimental/New feature or request Ivy Functional API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants