Skip to content

Small formatting fixes to make the spec parsable by the test suite scripts #133

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 12 commits into from
Apr 11, 2021

Conversation

asmeurer
Copy link
Member

No description provided.

__div__ is a relic from Python 2. It no longer does anything in Python 3.

The reflected operators were missing __rmatmul__. Now they completely match
the in-place operators.
@asmeurer
Copy link
Member Author

I also pushed a fix to the in-place and reflected operators.

I noticed we don't have __divmod__ (https://docs.python.org/3/reference/datamodel.html#emulating-numeric-types). I don't know if it is important (though it can be easily be naively implemented as __div__ + __mod__).

Also __pow__ is missing the optional third argument [modulo]. I don't know if we want to consider adding that as well. Certainly it should not be disallowed.

The rule said that empty slices should work, but the note right below that
says that they are optional.
@asmeurer
Copy link
Member Author

asmeurer commented Mar 5, 2021

Since this PR is still open, I also pushed a fix to an issue I noticed with the indexing spec. It was ambiguous whether empty slices are required or optional. I removed the text that said they were required and left in the note that said they are optional.

Assuming this is indeed what we want, should the NumPy array API namespace indexing support empty slices?

@@ -105,11 +105,6 @@ Slice syntax must have the following defaults. Let `n` be the axis (dimension) s
- If `k` is greater than `0` and `j` is not provided (e.g., `0::2`), `j` must equal `n`.
- If `k` is less than `0` and `i` is not provided (e.g., `:10:-2`), `i` must equal `n-1`.
- If `k` is less than `0` and `j` is not provided (e.g., `0::-2`), `j` must equal `-n-1`.

Using a slice to index a single array axis must adhere to the following rules. Let `n` be the axis (dimension) size.
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should not be removed. Otherwise, the item defined by L113 will look like it belongs to the list ending at L107.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well that was also confusing to me. The two lists both seem like they are about the same thing (the behavior of a slice on an axis of size n).

@kgryte
Copy link
Contributor

kgryte commented Mar 5, 2021

To split hairs a bit, the rule you removed was referring to the case where i == j; while the potential conflict arises in the note regarding out-of-bounds indexing (e.g., where an index exceeds dimensions). It is not clear to me that the two cases are the same.

@asmeurer
Copy link
Member Author

asmeurer commented Mar 5, 2021

Oh I think you are right. I will revert the commit.

@asmeurer
Copy link
Member Author

asmeurer commented Mar 5, 2021

I guess the reason it confused me is that this isn't the only way to get an empty slice. As long as the element indexed by stop is before the start (or after for negative step), the slice is empty. For example, a[3:1].

I'm unclear which cases we should allow or disallow in the NumPy array API. Should we disallow any slice that results in an empty selection except for the case where start == stop?

@rgommers rgommers added the Maintenance Bug fix, typo fix, or general maintenance. label Mar 20, 2021
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks @asmeurer and @kgryte

@rgommers rgommers merged commit f13b856 into data-apis:main Apr 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Bug fix, typo fix, or general maintenance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants