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

Adding annotations - triangle.py #522

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

genedan
Copy link
Contributor

@genedan genedan commented May 21, 2024

I'm mostly adding things like comments and type hints. Also fixes that would make the code compliant with PEP guidelines.

@MatthewCaseres
Copy link
Contributor

  1. Is there a way to avoid code like columns: str | list = None? If columns is str or list, how can it be None by default?
  2. What do the lists contain, strings?

@kennethshsu
Copy link
Collaborator

@genedan how about (origin: str or array-like = None, let's try to match pandas.DataFrame?

@MatthewCaseres is that actually an issue? The argument is not always used. Do you think that will cause confusion? I guess we could do (origin: str, array-like, or None, default None?

@MatthewCaseres
Copy link
Contributor

@kennethshsu Some arguments don't describe themselves as optional, but since they have a default None they must be optional. I was having some trouble understanding what I'm supposed to pass into the constructor, like can I omit the origin? Can I pass a list of dictionaries into the origin?

@genedan
Copy link
Contributor Author

genedan commented May 22, 2024

According to PEP 484 we need some explicit annotation to indicate a parameter is optional:

https://stackoverflow.com/questions/72039810/python-3-10-optional-parameter-type-union-vs-none-default

I'm thinking we could do something like:

columns: Optional[str | list] = None

or

columns: str | list | None = None

I would prefer the former since it's more explicit that the parameter is optional.

@genedan genedan changed the title Adding annotations Adding annotations - triangle.py May 22, 2024
@kennethshsu
Copy link
Collaborator

This looks really good, I like it a lot!

To @MatthewCaseres's point, I think we can do a better job with input validation. I'll add this to my backlog.

@kennethshsu
Copy link
Collaborator

On second look, I thought this is for the docstrings, not the actual declaration in the code. But good still!

@genedan
Copy link
Contributor Author

genedan commented May 23, 2024

What other types can be accepted for the data argument? The documentation has DataFrame, but we have a method _interchange_dataframe that allows for other types to be accepted that support the __dataframe__ protocol. So this implies that the triangle constructor is somewhat flexible.

So I was thinking the accepted type for data should be something like DataFrame, DataFrame-like but I'm not sure what Python types would be available for "DataFrame-like". Maybe we could use array-like but specify in the docstring that it needs to support the __dataframe__ protocol?

@genedan
Copy link
Contributor Author

genedan commented May 23, 2024

In the pandas source code, they annotate the input as DataFrameXchg which comes from a stripped down version of a DataFrame used for the conversion, so I'll go with that.

import pandas as pd
from pandas.core.interchange.dataframe_protocol import (
    Buffer,
    Column,
    ColumnNullType,
    DataFrame as DataFrameXchg,
    DtypeKind,
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants