Skip to content
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

builtin math shouldn't require a --scale argument for floating point math. #4478

Closed
floam opened this issue Oct 13, 2017 · 8 comments
Closed

builtin math shouldn't require a --scale argument for floating point math. #4478

floam opened this issue Oct 13, 2017 · 8 comments

Comments

@floam
Copy link
Member

@floam floam commented Oct 13, 2017

The old math function wouldn't do this (we cast to an integer in builtin_math.cpp):

~ $ math '3.5*3'
10

bc would print 10.5.

The only way to coax out the right answer is to use --scale, which I consider something kind of unpleasant from our old math function that was an artifact of bc, not something we should want to encourage let alone require. Ideally we could have a nice default where 5.0 printed 5 and 5.0010 printed 5.001 (roughly %g without the exponent stuff).

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Oct 14, 2017

Wow, that's gross. Floating point arithmetic ought to just work.

@floam
Copy link
Member Author

@floam floam commented Oct 14, 2017

Indeed, and I'd say that the scale argument should just go away. People can run math output through printf themselves if they want to tweak it.

@talamus
Copy link

@talamus talamus commented Feb 8, 2018

the math commandlet is a thin wrapper around bc, so

> math "scale=10; 100/3"

produces what expected.

However, if you set default scale for bc it seems to work with math also.
(There is a scale option in the commandlet code, but it seems to format the (already rounded) output.)

@faho
Copy link
Member

@faho faho commented Feb 8, 2018

the math commandlet is a thin wrapper around bc, so

@talamus: It's not anymore. In 3.0, it's a builtin.

And I don't think we want to add something like "scale". At least not in the way that bc does - without, you can only get integers, and with it, you can't do modulo.

@zanchey zanchey modified the milestones: fish-future, fish-3.0 Feb 9, 2018
@faho
Copy link
Member

@faho faho commented Mar 7, 2018

So, I've tried to just remove trailing zeroes and "." from the string represenation (unfortunately there's no useful standardized format string for this - %g is almost correct except it switches to 1e15 scientific notation if the numbers get big or small), and that looks okay.

math 5 / 5 results in "1", math 4 / 2 results in "2", math 2 / 3 results in 0.666667 (or something like it).

However, one issue I have here is that test doesn't handle floats, and I'd rather have truncating math with results I can feed to that than to always use math --integer with it.

@zanchey
Copy link
Member

@zanchey zanchey commented Mar 9, 2018

What about teaching test to handle floats? That seems like a sensible extension.

@faho
Copy link
Member

@faho faho commented Mar 9, 2018

Sure. I'd add one caveat: We always accept "." as the separator, so scripts don't break across locales just because of numeric crap (AFAICT, there is no POSIX way to read a double that isn't locale-dependent). I've added something like that in math, and I just ended up switching LC_NUMERIC temporarily. In test, it might be possible to accept both (in math it's not because "," is also used for multi-argument functions - what does ncr(2,5) mean?), but I'd lean towards accepting the same numbers for both.

(It's possible that fish should just use LC_NUMERIC=C internally always - which would make this much easier)

@faho
Copy link
Member

@faho faho commented Mar 9, 2018

Okay, printf explicitly tries the user's locale and then C.

@faho faho added enhancement and removed bug labels Mar 25, 2018
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Jul 24, 2018
This changes the behavior of builtin math to floating point by default.
If the result of a computation is an integer, then it will be printed as an
integer; otherwise it will be printed as a floating point decimal with up to
'scale' digits past the decimal point (default is 6, matching printf).
Trailing zeros are trimmed. Values are rounded following printf semantics.

Fixes fish-shell#4478
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Aug 4, 2018
This changes the behavior of builtin math to floating point by default.
If the result of a computation is an integer, then it will be printed as an
integer; otherwise it will be printed as a floating point decimal with up to
'scale' digits past the decimal point (default is 6, matching printf).
Trailing zeros are trimmed. Values are rounded following printf semantics.

Fixes fish-shell#4478
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants