Error loading a table from a gsheet

[schanzer]

Joy reported this from a teacher...

Run this program, then evaluate Cell-Table in the interactions area. You can load the spreadsheet linked from the Pyret file to check out what's going on.

It looks like Pyret's importing-from-google-sheets functionality is getting tripped up by extremely small values in google sheets.

[blerner]

The table is

Molecules	Species	Speed
Na+	ions	5.00E-04
K+	ions	4.70E-04
O2	small molecules	2.30E+12
CO2	small molecules	3.50E+08
H2O	small molecules	3.40E+04
EtOH	small molecules	2.10E+04
Steroids	small molecules	1.00E+04
Urea	small molecules	5.40E-02
Glycerol	small molecules	5.40E-02
small molecule drugs	small molecules	1.00E+00
Cyclosporin	peptides	2.50E-04
TAT	peptides	2.70E+04

And Pyret produces

Molecules	Species	Speed
"Na+"	"ions"	0.0005
"K+"	"ions"	0.00047
"O2"	"small molecules"	2300000000000
"CO2"	"small molecules"	350000000
"H2O"	"small molecules"	34000
"EtOH"	"small molecules"	21000
"Steroids"	"small molecules"	10000
"Urea"	"small molecules"	0.054
"Glycerol"	"small molecules"	0.054
"small molecule drugs"	"small molecules"	1
"Cyclosporin"	"peptides"	0.00025
"TAT"	"peptides"	27000

Which of those values look wrong to you?

[schanzer]

@blerner Are you able to run the program I linked, and evaluate Cell-Table? On my machine (up-to-date Brave, macOS, plenty of RAM) it dies with a cryptic "error displaying value"

[blerner]

I ran it and literally copied and pasted cpo's output. I see no problem on my machine. It runs in both Firefox and Chrome.

[schanzer]

@blerner oh crap - looks like the teacher tweaked the numbers to avoid the bug. Sorry, I didn't think they'd touch the file!

I re-made the file and spreadsheet, and edited the issue text to use the new link.

[blerner]

I see it now. This is a @ds26gte bug to fix: https://github.com/brownplt/code.pyret.org/blob/horizon/src/web/js/output-ui.js#L1402 passes in only three arguments to toRepeatingDecimal, but the definition at https://github.com/brownplt/pyret-lang/blob/horizon/src/js/base/js-numbers.js#L3910 expects four arguments.

Looking at the usages of this function: in pyret-lang, the options argument is always undefined:

blerner@blerner-x1:~/pyret-lang$ grep -rn "toRepeatingDecimal(" src/
src/runtime-arr-preludes/compiled/builtin/js-numbers.js:4020:  var decimal = toRepeatingDecimal(n.numerator(), n.denominator(), undefined, errbacks);
src/js/base/js-numbers.js:3967:    var decimal = toRepeatingDecimal(n.numerator(), n.denominator(), undefined, errbacks);
src/runtime-arr/compiled/builtin/js-numbers.js:4020:  var decimal = toRepeatingDecimal(n.numerator(), n.denominator(), undefined, errbacks);

and in cpo, both uses forget to pass the extra argument at all.

blerner@blerner-x1:~/code.pyret.org$ grep -rn "toRepeatingDecimal(" src/
src/web/js/output-ui.js:1402:          var decimal = jsnums.toRepeatingDecimal(numr, denr, runtime.NumberErrbacks);
src/web/js/ide.js:138:      var decimal = jsnums.toRepeatingDecimal(num.numerator(), num.denominator(), runtime.NumberErrbacks);

(@ds26gte , I know we've talked about this before: we really do need a thorough test suite of every possible trigger of every possible NumberErrback, to make sure we've threaded them through correctly in all call-sites...)

[schanzer]

@blerner while I agree that a thorough test suite would be ideal, I think we can get most of the way there simply by invoking the TypeScript compiler on a minimally-annotated version of the file. I spent a little time this morning doing that, and tsc immediately finds quite a few places where the wrong number of arguments are passed (almost all of them involve a missing errbacks argument!), not to mention some dead code and missing null checks.

GitHub won't let me attach a .ts or .tsx file, so I've zipped it here for you and @ds26gte . Dorai and I spoke about it just now, and he's going to take a look and see if this lets us pick a bunch of low-hanging fruit.

js-numbers.ts.zip

[blerner]

Whatever this type-annotated delta is over the original file, we should be able to write unit tests for each such modified call site, right?

[schanzer]

Presumably, but it may be hard to trace the series of calls through Pyret that make it through to the offending call sites. If we're planning on sticking with this number implementation for a LONG time, I'd suggest breaking it off as a separate repo and writing unit tests just for js-nums.

And if the thinking is that we're eventually moving to math.js in the next few years, I'd just pick the low-hanging fruit found by typescript for now.

[ds26gte]

fromFixnum() was failing for arguments that were less than about 1e-7. That is when the JS fixnum represenation starts using scientific notation, and fromFixnum() didn't handle it. I've added a fix via PR brownplt/pyret-lang#1810. (Tests included.)

Note that my PR #588 is still valid. It addresses the proper access of the errbacks value when there is an error of this sort, in order to produce an informative error message. This PR prevents the error itself.