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

✨ add pandas.Series #68

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Conversation

EkberHasanov
Copy link

@EkberHasanov EkberHasanov commented Jun 25, 2023

  • add pd.Series support to the pydantic_extra_types.pandas_types module
  • created tests for series type
  • add docs for basic usage

Selected Reviewer: @Kludex

@yezz123
Copy link
Collaborator

yezz123 commented Jun 29, 2023

Thanks for the initiative @EkberHasanov its look a good PR and worth to work in, as i can see the issue is regarding py37 and missing dependencies do you think its worth re-open it and work on it ?

@EkberHasanov
Copy link
Author

Thank you for your feedback, @yezz123 . I appreciate your positive remarks about the PR. I'm definitely interested in working on it further and ensuring that it works flawlessly without any remaining issues, but I don't know should I continue to change something and push it, because it failed so much and frustrated me.

@EkberHasanov EkberHasanov reopened this Jun 30, 2023
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e186814) to head (09e073d).
Report is 11 commits behind head on main.

❗ Current head 09e073d differs from pull request most recent head 811d664. Consider uploading reports for the commit 811d664 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##              main       #68    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           11         8     -3     
  Lines          685       552   -133     
  Branches       169       140    -29     
==========================================
- Hits           685       552   -133     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yezz123 yezz123 changed the title Type/pandas ✨ add pandas.Series Jun 30, 2023
@yezz123
Copy link
Collaborator

yezz123 commented Jun 30, 2023

please review

@yezz123 yezz123 linked an issue Jun 30, 2023 that may be closed by this pull request
Co-authored-by: Akbar <98239031+EkberHasanov@users.noreply.github.com>
Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

I'm unsure if we should make it possible to instantiate Series with __init__.

Maybe it's better to not have the __init__, and only implement the __get_pydantic_core_schema__.

pyproject.toml Outdated
@@ -39,14 +39,15 @@ classifiers = [
]
requires-python = '>=3.7'
dependencies = [
'pydantic>=2.0b3',
'pydantic>=2.0',
Copy link
Member

Choose a reason for hiding this comment

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

Please open a new PR for this.

Suggested change
'pydantic>=2.0',
'pydantic>=2.0b3',

pyproject.toml Outdated
]
dynamic = ['version']

[project.optional-dependencies]
all = [
'phonenumbers>=8,<9',
'pycountry>=22,<23',
'pandas==1.3.5',
Copy link
Member

Choose a reason for hiding this comment

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

Why not pandas>=2.0?

Copy link
Author

Choose a reason for hiding this comment

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

Because pandas>=2.0 requires python >=3.8



class Series:
def __init__(self, value: Any) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

What is this "value"? pd.Series accepts many arguments. 🤔

) -> core_schema.AfterValidatorFunctionSchema:
return core_schema.general_after_validator_function(
cls._validate,
core_schema.any_schema(),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the best we can do. Is it? A series should be an array of some type, and I guess we are able to get this type as well.

@@ -4,3 +4,4 @@ annotated-types
black
pyupgrade
ruff
pandas-stubs
Copy link
Member

Choose a reason for hiding this comment

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

Likely not needed on pandas >= 2.

@EkberHasanov
Copy link
Author

I'm unsure if we should make it possible to instantiate Series with __init__.

Maybe it's better to not have the __init__, and only implement the __get_pydantic_core_schema__.

Oh yes, I guess inheriting the Series class from pd.Series makes this implementation much cleaner and more aligned with the existing functionality of pd.Series. Regarding the __init__ method, you're absolutely right that it may not be necessary. Since we are inheriting from pd.Series, we can rely on its default behavior for instantiation.

Thank you for pointing out this alternative approach. I'm excited about this implementation as it provides a more elegant and intuitive way to work with series data using Pydantic. Thank you for your feedback!

@dzmitry-lahoda
Copy link

@EkberHasanov how it is going? should somebody catch up and finish this PR? thank you

@EkberHasanov
Copy link
Author

@EkberHasanov how it is going? should somebody catch up and finish this PR? thank you

Hi @dzmitry-lahoda it's all good, thanks! I have made some changes last time, but i guess i have to update library versions now. I will give it a try to update the versions of the libraries and see what is going on, and then i will let you you know if I need further help. Thanks

@EkberHasanov
Copy link
Author

Hi @dzmitry-lahoda, hope you're okay. I guess, I may need help to resolve all these issues. Could someone please help or perhaps explain what and where is wrong? I though I know, but it seems i'm wrong.

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.

Support pandas types
4 participants