Skip to content

Add parser for extended floating point numbers - #2363

Merged
mattdowle merged 9 commits into
masterfrom
fread-nans
Sep 16, 2017
Merged

Add parser for extended floating point numbers#2363
mattdowle merged 9 commits into
masterfrom
fread-nans

Conversation

@st-pasha

@st-pasha st-pasha commented Sep 15, 2017

Copy link
Copy Markdown
Contributor

Closes #1800

@st-pasha st-pasha added this to the v1.10.6 milestone Sep 15, 2017
@st-pasha st-pasha self-assigned this Sep 15, 2017
@st-pasha
st-pasha requested a review from mattdowle September 15, 2017 16:33
@codecov-io

codecov-io commented Sep 15, 2017 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2363 into master will increase coverage by 0.01%.
The diff coverage is 98.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2363      +/-   ##
==========================================
+ Coverage   91.23%   91.24%   +0.01%     
==========================================
  Files          61       61              
  Lines       11898    11929      +31     
==========================================
+ Hits        10855    10885      +30     
- Misses       1043     1044       +1
Impacted Files Coverage Δ
src/freadR.c 93.24% <ø> (ø) ⬆️
R/data.table.R 97.45% <100%> (ø) ⬆️
src/fread.c 95.72% <98.38%> (+0.02%) ⬆️

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 6140ef7...25d8886. Read the comment docs.

mattdowle
mattdowle requested changes Sep 15, 2017
View reviewed changes

@mattdowle mattdowle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some nice catches here (unrelated to the NaN change per se) which I'm liking and wanting in very much.
But unfortunately "NAN" input will be read as NA now since NAND has been commented out and NA_FLOAT64 used always. test() must be faulty in passing that and needs a fix to distinguish NA from NaN.

@mattdowle

mattdowle commented Sep 15, 2017

Copy link
Copy Markdown
Member

NB: fails are due to knitr/R-devel unrelated to this PR. master has the same problem will have to look at separately.

@st-pasha

st-pasha commented Sep 15, 2017 •
edited by mattdowle
Loading

Copy link
Copy Markdown
Contributor Author

Fixed the parser so that it returns NAND as before.
Added new issue #2365 to make sure that we fix the test() function at some point so that it can catch problems like these.

@mattdowle @st-pasha
Disabled knitr on Windows R-devel AppVeyor
72cbbe6 
Guessing that (very welcome) R-devel binary change on 12 Sep is the root cause. Temporary change so at least the rest of data.table can continue to be checked on R-devel on Windows.
@mattdowle @st-pasha
Added backtick=FALSE to pass latest R-devel
4f2005e
@mattdowle
Strengthened tests using identical
c71564d
@mattdowle

mattdowle commented Sep 16, 2017

Copy link
Copy Markdown
Member

After adding the stricter identical, one of them (1830.6) fails with a NA/NaN difference to be sorted out.

@mattdowle mattdowle mentioned this pull request Sep 16, 2017
Closed
@mattdowle
mattdowle merged commit 328c2fe into master Sep 16, 2017
@mattdowle
mattdowle deleted the fread-nans branch September 16, 2017 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattdowle mattdowle mattdowle approved these changes

Assignees

@st-pasha st-pasha

Labels

enhancement fread

Projects

None yet

Milestone

v1.11.0

Development

Successfully merging this pull request may close these issues.

3 participants

@st-pasha @codecov-io @mattdowle