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

TimedeltaArray freq validation without _from_sequence #24723

Merged
merged 3 commits into from
Jan 11, 2019

Conversation

jorisvandenbossche
Copy link
Member

This aligns TimedeltaArray.__init__ with DatetimeArray.__init__, using the same approach how @jbrockmendel now handled the freq validation in DatetimeArray in the merged #24686
First commits removes usage of from_sequence (from #24666), second commit adds the freq validation as in #24686

So related to the discussion at the end #24666 (and a partial revert of that). This makes the TimedeltaArray constructor again more restricted (only accepts correctly typed (int or timedelta64) containers.
Until we decide on the constructors in general (#24684), I would prefer to keep them strict: it is always easier to later expand functionality (and then also test it), than remove functionality.

@codecov
Copy link

codecov bot commented Jan 11, 2019

Codecov Report

Merging #24723 into master will decrease coverage by 49.31%.
The diff coverage is 64.7%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #24723       +/-   ##
===========================================
- Coverage   92.38%   43.07%   -49.32%     
===========================================
  Files         166      166               
  Lines       52321    52344       +23     
===========================================
- Hits        48337    22545    -25792     
- Misses       3984    29799    +25815
Flag Coverage Δ
#multiple ?
#single 43.07% <64.7%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/timedeltas.py 41.25% <64.7%> (-46.61%) ⬇️
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%) ⬇️
pandas/io/sas/sas7bdat.py 0% <0%> (-91.17%) ⬇️
pandas/io/sas/sas_xport.py 0% <0%> (-90.15%) ⬇️
... and 124 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 d46d2a5...19c5160. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 11, 2019

Codecov Report

Merging #24723 into master will increase coverage by <.01%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24723      +/-   ##
==========================================
+ Coverage   92.38%   92.38%   +<.01%     
==========================================
  Files         166      166              
  Lines       52321    52344      +23     
==========================================
+ Hits        48337    48360      +23     
  Misses       3984     3984
Flag Coverage Δ
#multiple 90.81% <94.11%> (ø) ⬆️
#single 43.07% <64.7%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/timedeltas.py 88.21% <94.11%> (+0.35%) ⬆️
pandas/util/testing.py 88.09% <0%> (+0.09%) ⬆️

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 d46d2a5...19c5160. Read the comment docs.

@TomAugspurger TomAugspurger added Datetime Datetime data dtype Timedelta Timedelta data type and removed Datetime Datetime data dtype labels Jan 11, 2019
@TomAugspurger
Copy link
Contributor

Doing this for the RC?

@jorisvandenbossche
Copy link
Member Author

I would personally do that, but it might need some discussion with @jreback / @jbrockmendel ?

@TomAugspurger
Copy link
Contributor

Yeah. Though I'm also comfortable with doing this after the RC is tagged, but before 0.24.0.

@jreback jreback added this to the 0.24.0 milestone Jan 11, 2019
@jreback
Copy link
Contributor

jreback commented Jan 11, 2019

looks fine. @jbrockmendel

@TomAugspurger
Copy link
Contributor

I think let's just do this now. Apologies for ramming this through @jbrockmendel.

@TomAugspurger TomAugspurger merged commit c1a81fe into pandas-dev:master Jan 11, 2019
@jorisvandenbossche jorisvandenbossche deleted the tda-init branch January 11, 2019 14:14
@jbrockmendel
Copy link
Member

No sweat; I'm eager to see the RC too.

This does re-add a lot of redundant code. If the only objection to the previous implementation was that it let too many dtypes through, then just disallow those in the same line that disallowed np.bool_

@jorisvandenbossche
Copy link
Member Author

This does re-add a lot of redundant code. If the only objection to the previous implementation was that it let too many dtypes through, then just disallow those in the same line that disallowed np.bool_

Looking back at the discussion, I think other concerns were performance / the sequence_to_td64 doing too much other work ?

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
…v#24723)

* TimedeltaArray freq validation without _from_sequence
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
…v#24723)

* TimedeltaArray freq validation without _from_sequence
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Timedelta Timedelta data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants