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.jq: naive abs/0 #2767

Merged
merged 2 commits into from
Jul 26, 2023
Merged

builtin.jq: naive abs/0 #2767

merged 2 commits into from
Jul 26, 2023

Conversation

pkoppstein
Copy link
Contributor

manual.yml explains that the def is naive, and mentions fabs, etc.

This PR supersedes #2676

manual.yml explains that the def is naive, and mentions fabs, etc.
examples:
- program: 'map(abs)'
input: '[-10, -1.1, -1e-1, 1000000000000000002, -1000000000000000002]'
output: ['[10,1.1,1e-1,10000000000000002,1e+18]']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
output: ['[10,1.1,1e-1,10000000000000002,1e+18]']
output: ['[10,1.1,0.1,1000000000000000002,1e+18]']

Copy link
Contributor

Choose a reason for hiding this comment

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

As the corner case, 0 should be listed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think -1000000000000000002 shouldn't be here. The behavior is not guaranteed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Added tests for 0 and -0
  • Removed the contingent test in manual.yml
  • Used "NOT prescriptive" annotation in jq.test

@nicowilliams
Copy link
Contributor

Remind me why we need this change?

@pkoppstein
Copy link
Contributor Author

pkoppstein commented Jul 25, 2023

Remind me why we need this change?

  1. Familiarity [*1]

  2. Expressiveness
    abs allows for the clear expression of intent, thus absolving the programmer from having to add a comment.

(When using length for abs I usually feel obliged to add a comment in any program intended to be maintainable or shared.)

For non-mathematicians, length is cognitively quite distinct from abs; and for mathematicians, |_| is usually (always?) pronounced in some other way (e.g. abs :-).

  1. The alternative, fabs, forces conversion to float.

And who knows, in the future, maybe length will become less polymorphic!

The real question, in my view, is whether abs should raise an error for non-numeric input.

[*1]
"Many programming languages have functions that calculate absolute values of numbers, either having the name abs or Abs."
https://en.wikibooks.org/wiki/C_Programming/stdlib.h/abs

Here's a ChatGPT listing:

Here is an expanded list of programming languages that include a built-in abs() function for computing the absolute value:

Python
C/C++
Java
JavaScript
Ruby
PHP
MATLAB
R
Swift
Kotlin
Go
C#
Julia
Perl
Lua
Rust
Scala
Scheme
Haskell
Tcl
Fortran
Ada
Lisp (Common Lisp)
Clojure
Groovy
PowerShell
VB.NET (Visual Basic .NET)
Dart
Elixir
F#
Crystal
OCaml
Swift
TypeScript
ActionScript
Delphi/Object Pascal
PL/SQL (Oracle's procedural language extension to SQL)

Also clarify non-prescriptive nature of some tests in jq.test
@nicowilliams nicowilliams merged commit 13fbe98 into jqlang:master Jul 26, 2023
29 checks passed
@nicowilliams
Copy link
Contributor

Thanks!

@pkoppstein
Copy link
Contributor Author

@nicowilliams - I was expecting more back-and-forth on this one! Thanks.

Any chance of making progress on fixing walk? (#2655)
The current (1.6) def is so lame ...

If you think #2655 is too radical for jq 1.7, I'll submit a different PR that doesn't do anything but make it more efficient by introducing a subfunction.

Thanks also for the great progress on binary strings, especially saving $string[$i] !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants