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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add reminder op for floats #229

wants to merge 1 commit into from

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
give(json!(null), "0 % 4000000001", json!(0));
give(json!(null), "2.1 % -2 | . * 1000 | round", json!(100));
give(json!(null), "2.1 % -1 | . * 1000 | round", json!(100));
give(json!(null), "2.1 % 0 | tojson", json!("null"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure i understand why this is needed, because the value is NaN?

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