Skip to content

CLN: Removed second version of xstrtod in parser_helper.h #25925

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

Merged
merged 4 commits into from
Apr 3, 2019

Conversation

anmyachev
Copy link
Contributor

@anmyachev anmyachev commented Mar 29, 2019

@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25925      +/-   ##
==========================================
- Coverage    91.8%   91.79%   -0.01%     
==========================================
  Files         174      174              
  Lines       52536    52536              
==========================================
- Hits        48231    48226       -5     
- Misses       4305     4310       +5
Flag Coverage Δ
#multiple 90.35% <ø> (ø) ⬆️
#single 41.88% <ø> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 75% <0%> (-12.5%) ⬇️
pandas/core/frame.py 96.79% <0%> (-0.12%) ⬇️
pandas/util/testing.py 90.63% <0%> (-0.11%) ⬇️

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 1d4c89f...d7c6ffa. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25925      +/-   ##
==========================================
- Coverage   91.81%    91.8%   -0.01%     
==========================================
  Files         175      175              
  Lines       52578    52578              
==========================================
- Hits        48273    48269       -4     
- Misses       4305     4309       +4
Flag Coverage Δ
#multiple 90.36% <ø> (ø) ⬆️
#single 41.9% <ø> (-0.08%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 75% <0%> (-12.5%) ⬇️
pandas/core/frame.py 96.79% <0%> (-0.12%) ⬇️

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 f0ba498...7d56a15. Read the comment docs.

@gfyoung gfyoung added Numeric Operations Arithmetic, Comparison, and Logical operations Internals Related to non-user accessible pandas implementation Clean labels Mar 30, 2019
@gfyoung gfyoung requested a review from jbrockmendel March 30, 2019 01:44
@gfyoung
Copy link
Member

gfyoung commented Mar 30, 2019

@anmyachev : Thanks for the PR!

Can you run asv benchmarks to see if performance is impacted with this de-duplication?

@jbrockmendel
Copy link
Member

AFAICT this looks fine, but I'm going to kick the review over to @chris-b1 who is better-versed in this area.

@gfyoung
Copy link
Member

gfyoung commented Mar 30, 2019

@jbrockmendel: Sounds good. Was just pinging for the modified dependency linking (given your thoughts interest in reorganizing internal dependencies to make it more DAG)

@anmyachev
Copy link
Contributor Author

@gfyoung after this run:
asv continuous -f 1.01 origin/master HEAD -e -b inference.MaybeConvertNumeric
result: BENCHMARKS NOT SIGNIFICANTLY CHANGED.
Same result for run asv continuous -f 1.01 origin/master HEAD -e -b io.csv

  • First run check perf of xstrtod with maybe_int != NULL
  • Second run check perf of xstrtod with maybe_int == NULL

@anmyachev anmyachev force-pushed the concat_xstrtod_implementations branch from ff2ae8c to 7d56a15 Compare March 30, 2019 20:08
@jreback
Copy link
Contributor

jreback commented Apr 3, 2019

@chris-b1 any comments here

@jreback jreback added this to the 0.25.0 milestone Apr 3, 2019
Copy link
Contributor

@chris-b1 chris-b1 left a comment

Choose a reason for hiding this comment

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

lgtm!

@jreback jreback merged commit d591ade into pandas-dev:master Apr 3, 2019
@jreback
Copy link
Contributor

jreback commented Apr 3, 2019

thanks @anmyachev

@anmyachev anmyachev deleted the concat_xstrtod_implementations branch April 4, 2019 08:12
anmyachev added a commit to anmyachev/pandas that referenced this pull request Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Internals Related to non-user accessible pandas implementation Numeric Operations Arithmetic, Comparison, and Logical operations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate xstrtod implementations
5 participants