Skip to content

Update guidance for output array dtype inference in full_like #274

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

Merged
merged 2 commits into from
Oct 4, 2021

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Sep 27, 2021

This PR

  • updates the guidance for output array dtype inference in full_like.

Discussion

In gh-167, guidance was changed (see gh-31) to infer the dtype from the fill_value. However, this seems to have been a mistake, as scalar values do not have an array data type (only int or float) and do not participate in type promotion.

This PR reverts this change, but also includes an explicit note that, if the fill_value is not of the same data type "kind" as the resolved output array data type, then behavior is left unspecified (e.g., if x.dtype is int64 and fill_value is float; e.g., fill_value = 3.5).

@kgryte kgryte added bug Something isn't working. API change Changes to existing functions or objects in the API. labels Sep 27, 2021
@asmeurer
Copy link
Member

CC @honno

@asmeurer
Copy link
Member

However, this seems to have been a mistake, as scalar values do not have an array data type (only int or float) and do not participate in type promotion.

FWIW, this matched the behavior of full(), which infers the data type from fill_value when dtype=None.

@kgryte
Copy link
Contributor Author

kgryte commented Sep 27, 2021

@asmeurer Correct, but full_like has the advantage of additional information needed to make a better guess as to the desired output array data type. So full should probably not serve as a basis for full_like.

@asmeurer
Copy link
Member

Also, just to clarify, for all the *_like functions, the dtype of x should be completely ignored if dtype is provided, even if it isn't compatible? For instance, ones_like(array(..., dtype=bool), dtype=int8) (this was another point that came up in data-apis/array-api-tests#18).

Copy link
Member

@asmeurer asmeurer left a comment

Choose a reason for hiding this comment

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

I've mentioned before that I never quite understood the point of the *_like functions, but it does seem reasonable to me that they should try to make an array as "like" x as possible. And more to the point, this now matches the NumPy behavior. So this looks good to me.

honno added a commit to honno/array-api-tests that referenced this pull request Sep 29, 2021
honno added a commit to honno/array-api-tests that referenced this pull request Sep 29, 2021
honno added a commit to honno/array-api-tests that referenced this pull request Sep 29, 2021
@kgryte kgryte modified the milestones: v2022, v2021 Oct 4, 2021
@kgryte
Copy link
Contributor Author

kgryte commented Oct 4, 2021

Also, just to clarify, for all the *_like functions, the dtype of x should be completely ignored if dtype is provided, even if it isn't compatible?

Yes, it should be ignored. Compatibility does not apply for the *_like functions, as the functions infer meta data about the array and do nothing with the actual values of x themselves.

I will update the guidance in a separate PR, however, to make this a bit more explicit.

@kgryte
Copy link
Contributor Author

kgryte commented Oct 4, 2021

This has been approved and simply reverts to previous behavior. Merging...

@kgryte kgryte merged commit 436c97f into main Oct 4, 2021
@kgryte kgryte deleted the update-full-like branch October 4, 2021 16:54
cr313 added a commit to cr313/test-array-api that referenced this pull request Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change Changes to existing functions or objects in the API. bug Something isn't working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants