Skip to content

fromFixnum(): fix for when argument fixnum is less than about 1e-7, brownplt/code.pyret.org#556 #1810

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.

Sign up for GitHub

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 6 commits into from
Sep 19, 2025
Merged

fromFixnum(): fix for when argument fixnum is less than about 1e-7, brownplt/code.pyret.org#556 #1810

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
10ba82d
fromFixnum(): fix for argument fixnum is less than about 1e-7, brownp…
ds26gte Aug 25, 2025
92aee48
fromFixnum() simplified, with better var names, and comments
ds26gte Aug 30, 2025
11cbc5c
- define fromFixnum() in terms of fromString()
ds26gte Sep 14, 2025
a9fb542
Merge branch 'horizon' into fromFixnum-fix
ds26gte Sep 19, 2025
777d118
scrub regexp no longer used
ds26gte Sep 19, 2025
b3311db
revert change to fromString() that replaces makeBignum() calls with r…
ds26gte Sep 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions src/js/base/js-numbers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,17 +173,36 @@ define("pyret-base/js/js-numbers", function() {
} else {
// used to return float, now rational
var stringRep = x.toString();
var match = stringRep.match(/^(.*)\.(.*)$/);
var match = stringRep.match(genScientificPattern);
var factor1 = 1;
if (match) {
var divideP = false;
stringRep = match[1];
var exponentPart = match[2];
if (exponentPart.match('^-')) {
divideP = true;
exponentPart = exponentPart.substring(1);
}
var exponentValue = makeBignum("1" + zfill(Number(exponentPart)));
if (divideP) {
factor1 = divide(1, exponentValue);
}
else {
factor1 = exponentValue;
}
}
match = stringRep.match(/^(.*)\.(.*)$/);
Copy link
Member

Choose a reason for hiding this comment

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

Can we call factor1 something like exponentFactor instead? That would have helped me a lot to understand why it's initialized to 1.

Copy link
Member

Choose a reason for hiding this comment

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

It would have also helped me to read the code if the match variable wasn't re-used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

var factor2;
Copy link
Member

Choose a reason for hiding this comment

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

Similarly could this be called baseFactor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (match) {
var afterDecimal = parseInt(match[2]);
var factorToInt = Math.pow(10, match[2].length);
var extraFactor = _integerGcd(factorToInt, afterDecimal);
var multFactor = factorToInt / extraFactor;
return Rational.makeInstance(Math.round(x*multFactor), Math.round(factorToInt/extraFactor), errbacks);
factor2 = Rational.makeInstance(Math.round(x*multFactor), Math.round(factorToInt/extraFactor), errbacks);
} else {
return Rational.makeInstance(x, 1, errbacks);
factor2 = Rational.makeInstance(Number(stringRep), 1, errbacks);
Copy link
Member

Choose a reason for hiding this comment

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

Can you help me understand this change? What's an example where using x would have been wrong? I can't think of one, but maybe I'm not thinking through the ranges correctly.

Copy link
Contributor Author

@ds26gte ds26gte Aug 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I had not updated x to be the "shorter" (i.e. sans exponent) number and hence had to use Number(stringRep).

In the most recent push, I've updated x and so can use Rational.makeInstance(x, 1, ...).

Copy link
Contributor Author

@ds26gte ds26gte Aug 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also, I found the older code dealing with the decimal point a bit opaque, so I've taken the liberty of using more expressive variable names and added comments to aid a future traveler. Hope that's ok.

}

return multiply(factor1, factor2);
}
};

Expand Down Expand Up @@ -2036,6 +2055,8 @@ define("pyret-base/js/js-numbers", function() {

var scientificPattern = new RegExp("^([+-]?\\d*\\.?\\d*)[Ee]([+]?\\d+)$");

var genScientificPattern = new RegExp("^([+-]?\\d*\\.?\\d*)[Ee]([+-]?\\d+)$");
Copy link
Member

Choose a reason for hiding this comment

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

Is scientificPattern used elsewhere? Is it wrong when used elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scientificPattern, which doesn't allow a negative exponent, is used by expandExponent(), which is used by makeBignum(). I don't know why a negative exponent is explicitly disallowed. It's true that makeBignum(), since it makes "big" integers, doesn't need to handle negative exponents.

For fromFixnum(), we definitely need to handle negative exponents. The bug for which this fix was made was about misreading numbers like 5e-19 from an Excel sheet. So the new regexp pattern had to be introduced.


// fromString: string -> (pyretnum | false)
var fromString = function(x, errbacks) {
if (x.match(digitRegexp)) {
Expand Down
3 changes: 3 additions & 0 deletions tests/pyret/tests/test-json.arr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ check "conversion":
p('[5, null, {"hello": "world"}]') is
J.j-arr([list: J.j-num(5), J.j-null,
J.j-obj([SD.string-dict: "hello", J.j-str("world")])])

p('1E-7').native() is 1e-7
p('5E-19').native() is 5e-19
end

check "native":
Expand Down
Loading