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-#4966: Fix to_timedelta to return Series instead of TimedeltaIndex #5028

Merged
merged 12 commits into from
Oct 4, 2022

Conversation

billiam-wang
Copy link
Collaborator

@billiam-wang billiam-wang commented Sep 23, 2022

Signed-off-by: Bill Wang billiam@ponder.io

What do these changes do?

Added implementation for to_timedelta instead of using pandas version. Previously returned incorrect output when using a Modin Series as input.

Added tests for to_timedelta with Modin Series.

  • commit message follows format outlined here
  • passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
  • passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
  • signed commit with git commit -s
  • Resolves BUG: calling pd.to_timedelta on modin series gives TimedeltaIndex instead of Series #4966
  • tests added and passing
  • module layout described at docs/development/architecture.rst is up-to-date
  • added (Issue Number: PR title (PR Number)) and github username to release notes for next major release

@billiam-wang billiam-wang requested a review from a team as a code owner September 23, 2022 18:14
@codecov
Copy link

codecov bot commented Sep 23, 2022

Codecov Report

Merging #5028 (8923ae3) into master (238a428) will decrease coverage by 10.30%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master    #5028       +/-   ##
===========================================
- Coverage   84.78%   74.48%   -10.31%     
===========================================
  Files         253      254        +1     
  Lines       19133    19428      +295     
===========================================
- Hits        16222    14470     -1752     
- Misses       2911     4958     +2047     
Impacted Files Coverage Δ
modin/core/storage_formats/base/query_compiler.py 99.23% <100.00%> (+<0.01%) ⬆️
...odin/core/storage_formats/pandas/query_compiler.py 96.40% <100.00%> (+0.56%) ⬆️
modin/pandas/general.py 96.38% <100.00%> (+0.11%) ⬆️
...odin/experimental/core/storage_formats/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
.../experimental/core/storage_formats/hdk/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
...ative/implementations/hdk_on_native/io/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
...mplementations/hdk_on_native/dataframe/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
...ementations/hdk_on_native/partitioning/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
...mplementations/hdk_on_native/calcite_serializer.py 0.00% <0.00%> (-98.71%) ⬇️
...e/implementations/hdk_on_native/calcite_builder.py 0.00% <0.00%> (-96.37%) ⬇️
... and 62 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@pyrito pyrito left a comment

Choose a reason for hiding this comment

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

@Billy2551 looks like there are some documentation errors. Could you take a look at them?

@mvashishtha
Copy link
Collaborator

looks like there are some documentation errors. Could you take a look at them?

@Billy2551 I recommend running locally the docstyle command from the failed CI job:

  python scripts/doc_checker.py --add-ignore=D101,D102,D103,D105 --disable-numpydoc \
    modin/pandas/dataframe.py modin/pandas/series.py \
    modin/pandas/groupby.py \
    modin/pandas/series_utils.py modin/pandas/general.py \
    modin/pandas/plotting.py modin/pandas/utils.py \
    modin/pandas/iterator.py modin/pandas/indexing.py \

modin/core/storage_formats/base/query_compiler.py Outdated Show resolved Hide resolved
modin/core/storage_formats/pandas/query_compiler.py Outdated Show resolved Hide resolved
modin/pandas/general.py Show resolved Hide resolved
@pyrito pyrito requested a review from a team September 28, 2022 15:52
modin/core/storage_formats/base/query_compiler.py Outdated Show resolved Hide resolved
modin/core/storage_formats/base/query_compiler.py Outdated Show resolved Hide resolved
modin/pandas/general.py Outdated Show resolved Hide resolved
modin/pandas/test/test_general.py Outdated Show resolved Hide resolved
mvashishtha
mvashishtha previously approved these changes Sep 28, 2022
Copy link
Collaborator

@mvashishtha mvashishtha left a comment

Choose a reason for hiding this comment

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

LGTM @Billy2551 ! Thank you.

pyrito
pyrito previously approved these changes Sep 28, 2022
modin/pandas/general.py Show resolved Hide resolved
modin/pandas/test/test_general.py Outdated Show resolved Hide resolved
mvashishtha
mvashishtha previously approved these changes Sep 30, 2022
Copy link
Collaborator

@mvashishtha mvashishtha left a comment

Choose a reason for hiding this comment

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

@Billy2551 I have a minor comment, but LGTM once @vnlitvinov 's comments are resolved.

modin/pandas/general.py Show resolved Hide resolved
mvashishtha
mvashishtha previously approved these changes Sep 30, 2022
Copy link
Collaborator

@mvashishtha mvashishtha left a comment

Choose a reason for hiding this comment

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

Thanks @Billy2551

pyrito
pyrito previously approved these changes Sep 30, 2022
vnlitvinov
vnlitvinov previously approved these changes Sep 30, 2022
Copy link
Collaborator

@vnlitvinov vnlitvinov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Billy2551!

@YarShev YarShev dismissed stale reviews from vnlitvinov, pyrito, and mvashishtha via 8923ae3 October 4, 2022 07:29
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 this pull request may close these issues.

BUG: calling pd.to_timedelta on modin series gives TimedeltaIndex instead of Series
5 participants