Skip to content

Avoid unnecessary use of _coerce_scalar_to_timedelta_type #24793

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 17, 2019
Merged

Avoid unnecessary use of _coerce_scalar_to_timedelta_type #24793

merged 2 commits into from
Jan 17, 2019

Conversation

jbrockmendel
Copy link
Member

In nearly all the places where its used, we can just use Timedelta instead. Way simpler, avoids using a private function.

jbrockmendel
jbrockmendel commented Jan 16, 2019
View reviewed changes
pandas/core/dtypes/cast.py
@@ -569,16 +569,14 @@ def coerce_to_dtypes(result, dtypes):
if len(result) != len(dtypes):
raise AssertionError("_coerce_to_dtypes requires equal len arrays")

from pandas.core.tools.timedeltas import _coerce_scalar_to_timedelta_type

def conv(r, dtype):
try:
Copy link
Member Author

Choose a reason for hiding this comment

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

btw an upcoming PR is going to make the try/except part of this function unnecessary.

@gfyoung gfyoung added Internals Related to non-user accessible pandas implementation Clean labels Jan 16, 2019
@codecov
Copy link

codecov bot commented Jan 17, 2019

Codecov Report

Merging #24793 into master will decrease coverage by 49.47%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #24793       +/-   ##
===========================================
- Coverage   92.38%   42.91%   -49.48%     
===========================================
  Files         166      166               
  Lines       52382    52379        -3     
===========================================
- Hits        48395    22478    -25917     
- Misses       3987    29901    +25914
Flag Coverage Δ
#multiple ?
#single 42.91% <33.33%> (-0.02%) ⬇️
Impacted Files Coverage Δ
pandas/core/dtypes/cast.py 48.67% <0%> (-40.06%) ⬇️
pandas/core/indexes/timedeltas.py 44.41% <0%> (-45.84%) ⬇️
pandas/core/computation/pytables.py 92.04% <100%> (-0.33%) ⬇️
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/core/categorical.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/tseries/converter.py 0% <0%> (-100%) ⬇️
pandas/io/formats/html.py 0% <0%> (-99.35%) ⬇️
pandas/core/groupby/categorical.py 0% <0%> (-95.46%) ⬇️
... and 125 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17a6bc5...b7b0169. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 17, 2019 •
edited
Loading

Codecov Report

Merging #24793 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24793      +/-   ##
==========================================
- Coverage   92.38%   92.38%   -0.01%     
==========================================
  Files         166      166              
  Lines       52382    52379       -3     
==========================================
- Hits        48395    48391       -4     
- Misses       3987     3988       +1
Flag Coverage Δ
#multiple 90.81% <66.66%> (-0.01%) ⬇️
#single 42.91% <33.33%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/computation/pytables.py 92.35% <100%> (-0.03%) ⬇️
pandas/core/dtypes/cast.py 88.7% <100%> (-0.02%) ⬇️
pandas/core/indexes/timedeltas.py 90.22% <100%> (-0.03%) ⬇️
pandas/util/testing.py 88.04% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17a6bc5...b7b0169. Read the comment docs.

@jreback jreback added this to the 0.24.0 milestone Jan 17, 2019
@jreback jreback merged commit 512830b into pandas-dev:master Jan 17, 2019
@jreback
Copy link
Contributor

jreback commented Jan 17, 2019

thanks, there is 1 last use in pytables I think can be removed?

@jbrockmendel jbrockmendel deleted the ct branch January 17, 2019 15:39
@jbrockmendel
Copy link
Member Author

There's one use left in tools.timedeltas (same place where it is defined) and a handful of tests. I'll see if we can remove it altogether

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
@jbrockmendel @Pingviinituutti
Avoid unnecessary use of coerce_scalar_to_timedelta_type (pandas-dev#…
7d915e7 
…24793)
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
@jbrockmendel @Pingviinituutti
Avoid unnecessary use of coerce_scalar_to_timedelta_type (pandas-dev#…
5d2a997 
…24793)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

Assignees
No one assigned
Labels
Clean Internals Related to non-user accessible pandas implementation
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jbrockmendel @jreback @jorisvandenbossche @gfyoung