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

fix: Return appropriate data type for time mean and median #14471

Merged
merged 6 commits into from
Apr 13, 2024

Conversation

mcrumiller
Copy link
Contributor

@mcrumiller mcrumiller commented Feb 13, 2024

Resolves #14470. Part of overall #13599.

This also moves one step closer to deprecating Series.dt.mean and Series.dt.median, as the base Series' mean and median now support time dtypes.

Using the example from the issue, all cases now properly output a duration:

06:00:00
02:00:00
06:00:00
02:00:00
shape: (1, 2)
┌──────────┬──────────┐
│ mean     ┆ median   │
│ ---      ┆ ---      │
│ time     ┆ time     │
╞══════════╪══════════╡
│ 06:00:00 ┆ 02:00:00 │
└──────────┴──────────┘
shape: (1, 2)
┌──────────┬──────────┐
│ mean     ┆ median   │
│ ---      ┆ ---      │
│ time     ┆ time     │
╞══════════╪══════════╡
│ 06:00:00 ┆ 02:00:00 │
└──────────┴──────────┘

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Feb 13, 2024
@mcrumiller mcrumiller marked this pull request as ready for review February 13, 2024 20:00
Copy link
Collaborator

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

looks good, just some minor comments

py-polars/tests/unit/namespaces/test_datetime.py Outdated Show resolved Hide resolved
@mcrumiller mcrumiller marked this pull request as draft February 23, 2024 17:37
@mcrumiller mcrumiller marked this pull request as ready for review February 23, 2024 18:09
@mcrumiller
Copy link
Contributor Author

mcrumiller commented Feb 23, 2024

@MarcoGorelli I believe I've resolved all of your issues with minor test improvements:

  • Added Duration and Time to the regular Series.mean and Series.median checks (not in the dt namespace). I am not sure if these tests should be moved out of the namespace test, but I think they still logically belong, even if they are not technically part of the namespace anymore. When we get Date in here, we can deprecate the dt.mean and dt.median and, after that, move them over to the regular series tests.
  • Parametrized time unit where applicable
  • Added a few None cases to existing parametrizations
  • Clarified the id of certain parametrizations
  • Added an additional aggregate median test to match the existing aggregate mean test.

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 81.16%. Comparing base (dcee934) to head (e2cfc2a).
Report is 4 commits behind head on main.

Files Patch % Lines
...s-core/src/frame/group_by/aggregations/dispatch.rs 50.00% 2 Missing ⚠️
py-polars/polars/series/datetime.py 33.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14471      +/-   ##
==========================================
+ Coverage   81.14%   81.16%   +0.01%     
==========================================
  Files        1363     1368       +5     
  Lines      175282   175417     +135     
  Branches     2527     2527              
==========================================
+ Hits       142236   142379     +143     
+ Misses      32568    32561       -7     
+ Partials      478      477       -1     

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

Copy link
Collaborator

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

minor typing comment, but I think this looks good

thanks for sticking with these!

py-polars/tests/unit/namespaces/test_datetime.py Outdated Show resolved Hide resolved
py-polars/tests/unit/namespaces/test_datetime.py Outdated Show resolved Hide resolved
py-polars/tests/unit/namespaces/test_datetime.py Outdated Show resolved Hide resolved
py-polars/tests/unit/namespaces/test_datetime.py Outdated Show resolved Hide resolved
@mcrumiller mcrumiller marked this pull request as draft February 23, 2024 19:25
Copy link
Collaborator

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks @mcrumiller !

@mcrumiller mcrumiller marked this pull request as ready for review February 23, 2024 19:39
@ritchie46 ritchie46 merged commit 4b1b945 into pola-rs:main Apr 13, 2024
24 checks passed
@mcrumiller mcrumiller deleted the time-mean-median branch April 13, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mean and median for pl.Time should return a time
3 participants