-
Notifications
You must be signed in to change notification settings - Fork 967
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
fwrite gains scipen #3716
fwrite gains scipen #3716
Conversation
77a7520
to
0711e4a
Compare
…way from scientific notation minor adjustment more minor stuff remove unrelated test
0711e4a
to
d01dbc1
Compare
Codecov Report
@@ Coverage Diff @@
## master #3716 +/- ##
==========================================
+ Coverage 99.42% 99.42% +<.01%
==========================================
Files 72 72
Lines 13498 13502 +4
==========================================
+ Hits 13421 13425 +4
Misses 77 77
Continue to review full report at Codecov.
|
@MichaelChirico There is |
Thanks @st-pasha... very good point to raise. I don't think it can become arbitrarily large -- should be at most But where
I guess |
Great. Could |
I was 50/50 on the naming, wanted to come up with something even more straightforward but couldn't. I honestly never knew what scipen stood for until working on this & it finally clicked. was hoping spelling it out could have that same effect on users. but not married to it. OTOH linking to ?options & the explanation in Details should also do the trick. |
It's nice that you've added it as an argument at all. So then new users know there's something there. In base read.csv and read.table it's not an argument, so you have to know a global option exists. That's what I thought we'd do actually: just make fwrite respect the global option without adding the argument. But adding the argument is better. |
Sure, done. I think adding it as an argument is in keeping with our aversion to Latest attempts to address @st-pasha's concern, I'm not sure how to test it though, can have a look? |
inst/tests/tests.Rraw
Outdated
test(2112.04, fwrite(DT, scipen=1), output=c("a,b,c","0.0001,1e+06,-20")) | ||
test(2112.05, fwrite(DT, scipen=2), output=c("a,b,c","0.0001,1000000,-20")) | ||
test(2112.06, fwrite(DT, scipen=-3), output=c("a,b,c","1e-04,1e+06,-20")) | ||
# pending ... test(2112.07, fwrite(DT, scipen=-4), output=c("a,b,c","1e-04,1e+06,-2e+01")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any comment why this is disabled would be nice
also scipen=0 test would be god
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found 2112.07 didn't work yesterday and will continue today. scipen=0 is test 2112.01.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I see what I was going for with the -1^(i+j)
tests... seems negatives not working:
for (i in -10:10) {cat(i, ':', sep = ''); fwrite(DT, scipen = i, col.names = FALSE)}
-10:1e-04,1e+06,-20
-9:1e-04,1e+06,-20
-8:1e-04,1e+06,-20
-7:1e-04,1e+06,-20
-6:1e-04,1e+06,-20
-5:1e-04,1e+06,-20
-4:1e-04,1e+06,-20
-3:1e-04,1e+06,-20
-2:1e-04,1e+06,-20
-1:1e-04,1e+06,-20
0:1e-04,1e+06,-20
1:0.0001,1e+06,-20
2:0.0001,1000000,-20
3:0.0001,1000000,-20
4:0.0001,1000000,-20
5:0.0001,1000000,-20
6:0.0001,1000000,-20
7:0.0001,1000000,-20
8:0.0001,1000000,-20
9:0.0001,1000000,-20
10:0.0001,1000000,-20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, but DT$c
is integer
. Implementation only applied scipen
to numeric... write.csv
also ignores scipen
for integer
(which is what I was getting at by comparing to format
, which write.csv
will match):
options(scipen = -4)
write.csv(DT)
"","a","b","c"
"1",1e-04,1e+06,-20
Closes #2020
via #3189
base
R handlesscipen
informat.c
here:The basic logic is in place (and actually quite simple as we already had a branch in place that did everything but use the scientific penalty), but some details of implementation should be solved in this PR before writing the NEWS/manual entry. I haven't quite convinced myself that
writeFloat64
is working exactly the same for scientific-vs-decimal format but I haven't found any counterexamples yet.As it is now, I guess it's not backwards-compatible (although tests passed) -- essentially, we moved from a default of
scientificPenalty=0
toscientificPenalty=getOption('scipen', 0L)
. I think the default value is 0, which is why tests passed on my machine. Shall we phase this in through deprecation cycle?Next, it seems
fwrite
handles overflow better (haven't dug into why, and not sure it's relevant, but it's worth noting now that we'll support writing very wide decimals):Once we agree on the details of how to proceed, we can add some more tests & tidy up before merging.
Assigning @st-pasha for review since we added to
fwriteMainArgs
, which I guess would affect pydatatable as well.Feedback welcome.