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

Add reminder op for floats #229

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Add reminder op for floats #229

merged 1 commit into from
Nov 25, 2024

Conversation

wader
Copy link
Contributor

@wader wader commented Nov 11, 2024

This is same as jq modulo (hoh) error message:

$ jq -n 'def f: -2, -1, 0, 2.1, 3, 4000000001; [f as $a | f as $b | try ($a % $b) catch .]'
[
  0,
  0,
  "number (-2) and number (0) cannot be divided (remainder) because the divisor is zero",
  0,
  -2,
  -2,
  -1,
  0,
  "number (-1) and number (0) cannot be divided (remainder) because the divisor is zero",
  -1,
  -1,
  -1,
  0,
  0,
  "number (0) and number (0) cannot be divided (remainder) because the divisor is zero",
  0,
  0,
  0,
  0,
  0,
  "number (2.1) and number (0) cannot be divided (remainder) because the divisor is zero",
  0,
  2,
  2,
  1,
  0,
  "number (3) and number (0) cannot be divided (remainder) because the divisor is zero",
  1,
  0,
  3,
  1,
  0,
  "number (4000000001) and number (0) cannot be divided (remainder) because the divisor is zero",
  1,
  2,
  0
]

@wader
Copy link
Contributor Author

wader commented Nov 11, 2024

Seems CI failure is same as what main branch is failing on

@01mf02
Copy link
Owner

01mf02 commented Nov 13, 2024

In accordance with what I wrote in #232 (comment), I think that the behaviour in this PR is not desirable, because it silently loses information about the decimal points. I'm sorry. I hope you understand.

@wader
Copy link
Contributor Author

wader commented Nov 13, 2024

No worries :) and the reason why jaq does not do non-integer reminder is because jq lacks it? i wonder if jaq's README or documentation could benefit from some notes and gotchas about how to do integer reminder etc, ex using floor etc?

@01mf02
Copy link
Owner

01mf02 commented Nov 13, 2024

Hmmm ... we could actually implement non-integer remainders quite easily, because we can already do l % r in Rust for floating-point numbers l and r, see: https://doc.rust-lang.org/std/primitive.f64.html#impl-Rem-for-f64
That means that when we calculate l % r and either l or r are float, then the other argument should be converted to a float as well and floating-point remainder should be calculated.
I think that if you would adapt your PR that way, I would accept it.

i wonder if jaq's README or documentation could benefit from some notes and gotchas about how to do integer reminder etc, ex using floor etc?

I thought that the "Numbers" section of the README did already contain this information. If you have a specific text in mind, I'm all for a little PR. :)

@wader
Copy link
Contributor Author

wader commented Nov 13, 2024

Hmmm ... we could actually implement non-integer remainders quite easily, because we can already do l % r in Rust for floating-point numbers l and r, see: https://doc.rust-lang.org/std/primitive.f64.html#impl-Rem-for-f64 That means that when we calculate l % r and either l or r are float, then the other argument should be converted to a float as well and floating-point remainder should be calculated. I think that if you would adapt your PR that way, I would accept it.

Ah ok! yeap i can have a look at that

i wonder if jaq's README or documentation could benefit from some notes and gotchas about how to do integer reminder etc, ex using floor etc?

I thought that the "Numbers" section of the README did already contain this information. If you have a specific text in mind, I'm all for a little PR. :)

Oh great 👍 not sure how i didn't see that

@wader
Copy link
Contributor Author

wader commented Nov 13, 2024

Switch float, but i'm not sure i really follow how reminder and floats work :) have to read more about it

$ cargo run -- -n -- '-2 / 2.1, -2 % 2.1'
-0.9523809523809523
-2.0

jaq-json/tests/funs.rs Outdated Show resolved Hide resolved
@wader wader changed the title Make reminder be integer reminder Add float reminder Nov 13, 2024
@wader wader changed the title Add float reminder Add reminder op for floats Nov 13, 2024
jaq-json/tests/funs.rs Outdated Show resolved Hide resolved
@01mf02 01mf02 merged commit 1a4b0c2 into 01mf02:main Nov 25, 2024
1 check passed
@01mf02
Copy link
Owner

01mf02 commented Nov 25, 2024

That's looking good now; thanks again for your PR!

@wader wader deleted the math-int-rem branch November 25, 2024 15:13
@01mf02 01mf02 mentioned this pull request Nov 25, 2024
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.

2 participants