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

remove unused args etal from np_datetime.c #18567

Merged
merged 1 commit into from
Dec 4, 2017

Conversation

jbrockmendel
Copy link
Member

What #18565 does for np_datetime_strings.c, this does for np_datetime.c

Getting rid of meta also gets rid of a potential problem noted in a comment that meta->num could potentially cause an overflow. Since meta->num is 1 in all pandas usages, that pitfall doesn't apply.

@codecov
Copy link

codecov bot commented Nov 29, 2017

Codecov Report

Merging #18567 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18567      +/-   ##
==========================================
+ Coverage   91.44%   91.45%   +<.01%     
==========================================
  Files         157      157              
  Lines       51449    51449              
==========================================
+ Hits        47048    47051       +3     
+ Misses       4401     4398       -3
Flag Coverage Δ
#multiple 89.32% <ø> (+0.02%) ⬆️
#single 40.6% <ø> (-0.11%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.81% <0%> (-0.1%) ⬇️
pandas/plotting/_converter.py 65.25% <0%> (+1.81%) ⬆️

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 bd9a3e0...c9c643d. Read the comment docs.

return result;
}

void pandas_datetime_to_datetimestruct(npy_datetime val, PANDAS_DATETIMEUNIT fr,
pandas_datetimestruct *result) {
pandas_datetime_metadata meta;
Copy link
Contributor

Choose a reason for hiding this comment

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

could remove these functions entirely (and just call the convert_*) ones?

Copy link
Member Author

Choose a reason for hiding this comment

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

Eventually. There are still some calls from np_datetime_strings.c and ujson/python/objToJSON.c so it isn't entirely trivial.

@jbrockmendel
Copy link
Member Author

ping. would like to close (most) existing before doing anything new.

@jreback
Copy link
Contributor

jreback commented Dec 3, 2017

@jbrockmendel yep that is fine. just rebase & ping on anything you'd like me to review. (so rebase here).

@jbrockmendel
Copy link
Member Author

just rebased+pushed; no new edits.

@jbrockmendel
Copy link
Member Author

should be good to go. #18565 may need rebasing after this; if not then it should be good to go too.

@jreback jreback added this to the 0.22.0 milestone Dec 4, 2017
@jreback jreback merged commit 02e72ec into pandas-dev:master Dec 4, 2017
@jreback
Copy link
Contributor

jreback commented Dec 4, 2017

thanks!

@jbrockmendel jbrockmendel deleted the npcruft3 branch December 8, 2017 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Datetime Datetime data dtype
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants