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

The behaviour of the overflow in multiply_pow10 #2297

Open
younies opened this issue Jul 31, 2022 · 2 comments
Open

The behaviour of the overflow in multiply_pow10 #2297

younies opened this issue Jul 31, 2022 · 2 comments
Assignees
Labels
C-numbers Component: Numbers, units, currencies S-small Size: One afternoon (small bug fix or enhancement) T-core Type: Required functionality

Comments

@younies
Copy link
Member

younies commented Jul 31, 2022

when a user call multiply_pow10 in FixedDecimal and an overflow happen, what shall we do:

  1. Set the number to Zero
  2. cut from the number (for example: if the number is 12345 and the user called multiply_pow10 by i16::MAX -3, the resulted number will be 34500000000......
  3. Add an infinity field to FixedDecimal so that +Infinity and -Infinity can be represented, and use that value when overflowing
  4. Saturate to the theoretical maximum FixedDecimal value
@younies younies added discuss Discuss at a future ICU4X-SC meeting C-numbers Component: Numbers, units, currencies good first issue Good for newcomers S-small Size: One afternoon (small bug fix or enhancement) labels Jul 31, 2022
@sffc
Copy link
Member

sffc commented Jun 22, 2023

Discuss with:

Optional:

@sffc sffc added the discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band label Jun 22, 2023
@sffc
Copy link
Member

sffc commented Jul 7, 2023

  • @younies - We need +/- infinity for units conversion and formatting
  • @eggrobin - There is CLDR data for infinity
  • @skius - Do we want differences between debug and release mode?
  • @Manishearth - Since this behaves sort-of like an f32 or f64, we can say that it just has an infinity value. We can say that infinity is a valid value to have. But maybe for overflow... ?
  • @younies - We need to find a way to set infinity. And do we need to differentiate between pow(1e6) and infinity?
  • @sffc - It's not normal to overflow as it is in f32/f64. So I think we should debug assert. But it's fine when creating directly from infinity input values.
  • @sffc - How about NaN?
  • @younies - We could make NaN the value when overflow occurs?
  • @eggrobin - That sounds like a new arithmetic standard
  • @Manishearth - NaN is specifically an IEEE thing. We are not IEEE floats. There is a reason to have a NaN-like value, but we don't expose these as things for people to manipulate. This is primarily for formatting. NaNs are weird.
  • @younies - What if you have infinity + infinity? So maybe infinity and overflow (and also NaN) should be different values. Then the user can know whether there was a problem.
  • @sffc - For now, I think we should limit the scope of this to infinity, but we should keep the door open for NaN only because then we can support infallible conversion from f64.
  • @sffc - We don't support arithmetic on FixedDecimal. We can debug assert to inform people that there was a problem. I don't think we should encode that in the data model.
  • @Manishearth - I don't think we should have a sentinel value. We should debug-assert and put in a sensical value in its place.
  • @younies - Is NaN formattable by CLDR?
  • @sffc - Yes
  • @eggrobin - I agree that this is not an arithmetic type, but to the extent that it has limited arithmetic, we should stick with an existing arithmetic standard.

Consensus: add Infinity and -Infinity to the FixedDecimal data model. Support it when reading from f32, f64, and strings (the string value is "Infinity" and "-Infinity"). Support infinity in FixedDecimalFormatter as a replacement for the # in the pattern, producing outputs such as "$∞K". When a FixedDecimal operation overflows into infinity, have a debug assertion.

LGTM: @younies @Manishearth @skius @eggrobin @sffc

@sffc sffc added T-core Type: Required functionality and removed good first issue Good for newcomers discuss Discuss at a future ICU4X-SC meeting discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band labels Jul 7, 2023
@sffc sffc added this to the 1.x Priority ⟨P2⟩ milestone Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-numbers Component: Numbers, units, currencies S-small Size: One afternoon (small bug fix or enhancement) T-core Type: Required functionality
Projects
None yet
Development

No branches or pull requests

2 participants