Skip to content

CLEAN: Resolve Axis and AxisType #284

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

Closed
Dr-Irv opened this issue Sep 9, 2022 · 12 comments · Fixed by #561
Closed

CLEAN: Resolve Axis and AxisType #284

Dr-Irv opened this issue Sep 9, 2022 · 12 comments · Fixed by #561

Comments

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Sep 9, 2022

In _typing.pyi, we have:

Axis = Union[str, int]
SeriesAxisType = Literal["index", 0]  # Restricted subset of _AxisType for series
AxisType = Literal["columns", "index", 0, 1]

There are 23 places (assuming I did grep correctly) where we use Axis, but can probably change this to AxisType in many (if not all) of them.

@ramvikrams
Copy link
Contributor

Now I think there is only a single place where Axis is used which is this Axis: TypeAlias = str | int

@Dr-Irv
Copy link
Collaborator Author

Dr-Irv commented Feb 27, 2023

Now I think there is only a single place where Axis is used which is this Axis: TypeAlias = str | int

The issue is that within the stubs themselves we have many places where we use Axis, so that is the reconciliation that needs to be done, especially with the introduction of AxisInt

@ramvikrams
Copy link
Contributor

Now I think there is only a single place where Axis is used which is this Axis: TypeAlias = str | int

The issue is that within the stubs themselves we have many places where we use Axis, so that is the reconciliation that needs to be done, especially with the introduction of AxisInt

yeah i'll do it

@ramvikrams
Copy link
Contributor

Actually i am not able to understand what is to be done with Axis in stubs because there are not many places where we can resolve Axis as it is declared it is mostly in inside funcs, but what is to be done I am not able to understand

@Dr-Irv
Copy link
Collaborator Author

Dr-Irv commented Feb 27, 2023

Actually i am not able to understand what is to be done with Axis in stubs because there are not many places where we can resolve Axis as it is declared it is mostly in inside funcs, but what is to be done I am not able to understand

In _typing.pyi, we have the following:

AxisInt: TypeAlias = Literal[0, 1]
Axis: TypeAlias = AxisInt | Literal["index", "columns", "rows"]
SeriesAxisType: TypeAlias = Literal[
    "index", 0
]  # Restricted subset of _AxisType for series
AxisTypeIndex: TypeAlias = Literal["index", 0]
AxisTypeColumn: TypeAlias = Literal["columns", 1]
AxisType: TypeAlias = AxisTypeIndex | AxisTypeColumn

In the pandas source, we use Axis as an argument type, while in pandas-stubs, we use AxisType. We should decide to go one way or the other, maybe by changing pandas.

Based on a discussion in pandas-dev/pandas#48612 , I'd like @twoertwein to comment on which way he thinks we should go within pandas-stubs.

@twoertwein
Copy link
Member

twoertwein commented Feb 27, 2023

I like the idea of separating Series-only/DataFrame axis values! Making that change in pandas will probably take some effort (especially for the integer indexing).

I'm slightly shocked how many Axis-related types we have, can we simplify it a bit. Maybe:

AxisInt: TypeAlias = Literal[0, 1]  # might not even be needed for the public API, typically used in private functions
AxisIndex: TypeAlias = Literal["index", 0]
AxisColumn: TypeAlias = Literal["columns", 1]
Axis: TypeAlias = AxisTypeIndex | AxisTypeColumn

@Dr-Irv
Copy link
Collaborator Author

Dr-Irv commented Feb 27, 2023

I like the idea of separating Series-only/DataFrame axis values! Making that change in pandas will probably take some effort (especially for the integer indexing).

I'm slightly shocked how many Axis-related types we have, can we simplify it a bit. Maybe:

AxisInt: TypeAlias = Literal[0, 1]  # might not even be needed for the public API, typically used in private functions
AxisIndex: TypeAlias = Literal["index", 0]
AxisColumn: TypeAlias = Literal["columns", 1]
Axis: TypeAlias = AxisTypeIndex | AxisTypeColumn

What about including "rows" in AxisIndex ?

@twoertwein
Copy link
Member

What about including "rows" in AxisIndex ?

the pandas PR added it only because it is technically possible and to avoid a mypy error (but I think it is not very well documented/used). I'd be fine adding/excluding it.

@Dr-Irv
Copy link
Collaborator Author

Dr-Irv commented Feb 27, 2023

@ramvikrams
So the goal would be to use these:

AxisInt: TypeAlias = Literal[0, 1]  # might not even be needed for the public API, typically used in private functions
AxisIndex: TypeAlias = Literal["index", 0]
AxisColumn: TypeAlias = Literal["columns", 1]
Axis: TypeAlias = AxisTypeIndex | AxisTypeColumn

and remove AxisType, SeriesAxisType and AxisType (changing where we have AxisType to the "new" Axis)

We can remove AxisInt - it is only used in algorithms.take(), and just use a Literal[0, 1] there.

So that would mean we would only see AxisIndex, AxisColumn and Axis throughout the stubs.

@ramvikrams
Copy link
Contributor

@ramvikrams So the goal would be to use these:

AxisInt: TypeAlias = Literal[0, 1]  # might not even be needed for the public API, typically used in private functions
AxisIndex: TypeAlias = Literal["index", 0]
AxisColumn: TypeAlias = Literal["columns", 1]
Axis: TypeAlias = AxisTypeIndex | AxisTypeColumn

and remove AxisType, SeriesAxisType and AxisType (changing where we have AxisType to the "new" Axis)

We can remove AxisInt - it is only used in algorithms.take(), and just use a Literal[0, 1] there.

So that would mean we would only see AxisIndex, AxisColumn and Axis throughout the stubs.

thanks, i'll get started now

@ramvikrams
Copy link
Contributor

We don't have AxisInt but we have Axis: TypeAlias = str | int so should we remove this Axis to move forward with the orignal plan

@Dr-Irv
Copy link
Collaborator Author

Dr-Irv commented Mar 1, 2023

We don't have AxisInt but we have Axis: TypeAlias = str | int so should we remove this Axis to move forward with the orignal plan

Yes, the goal is to use what is now AxisType and make that Axis

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 a pull request may close this issue.

3 participants