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.

Sign up for GitHub

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

Improvements to FixedDecimal f64 APIs #1718

Merged
merged 2 commits into from
Mar 24, 2022
Merged

Improvements to FixedDecimal f64 APIs #1718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42cd434
DoublePrecision::Floating + improve docs
sffc Mar 22, 2022
33d6849
Rename function
sffc Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ffi/diplomat/src/fixed_decimal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub mod ffi {
/// See [the Rust docs](https://unicode-org.github.io/icu4x-docs/doc/fixed_decimal/decimal/struct.FixedDecimal.html#method.from_f64) for more information.
pub fn create_from_f64_with_max_precision(f: f64) -> Option<Box<ICU4XFixedDecimal>> {
Some(Box::new(ICU4XFixedDecimal(
FixedDecimal::new_from_f64(f, DoublePrecision::Maximum).ok()?,
FixedDecimal::try_from_f64(f, DoublePrecision::Floating).ok()?,
)))
}

Expand All @@ -57,7 +57,7 @@ pub mod ffi {
rounding_mode: ICU4XFixedDecimalRoundingMode,
) -> Option<Box<ICU4XFixedDecimal>> {
Some(Box::new(ICU4XFixedDecimal(
FixedDecimal::new_from_f64(
FixedDecimal::try_from_f64(
f,
DoublePrecision::Magnitude(precision, rounding_mode.into()),
)
Expand All @@ -74,7 +74,7 @@ pub mod ffi {
rounding_mode: ICU4XFixedDecimalRoundingMode,
) -> Option<Box<ICU4XFixedDecimal>> {
Some(Box::new(ICU4XFixedDecimal(
FixedDecimal::new_from_f64(
FixedDecimal::try_from_f64(
f,
DoublePrecision::SignificantDigits(digits, rounding_mode.into()),
)
Expand Down
50 changes: 34 additions & 16 deletions utils/fixed_decimal/src/decimal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -882,7 +882,7 @@ pub enum DoublePrecision {
///
/// This results in a FixedDecimal having enough digits to recover the original floating point
/// value, with no trailing zeros.
Maximum,
Floating,
}

/// Specifies how numbers should be rounded
Expand All @@ -901,27 +901,44 @@ pub enum RoundingMode {

#[cfg(feature = "ryu")]
impl FixedDecimal {
/// Construct a [`FixedDecimal`] from an f64. This uses `ryu` and
/// goes through an intermediate string representation, so is not
/// fully efficient. See [icu4x#166](https://github.com/unicode-org/icu4x/issues/166) for
/// more details.
/// Construct a [`FixedDecimal`] from an f64.
///
/// Since f64 values do not carry a notion of their precision, the second argument to this
/// function specifies the type of precision associated with the f64. For more information,
/// see [`DoublePrecision`].
///
/// This function uses `ryu`, which is an efficient double-to-string algorithm, but other
/// implementations may yield higher performance; for more details, see
/// [icu4x#166](https://github.com/unicode-org/icu4x/issues/166).
///
/// This function can be made available with the `"ryu"` feature.
///
/// ```rust
/// use fixed_decimal::{DoublePrecision, FixedDecimal};
/// use fixed_decimal::{DoublePrecision, FixedDecimal, RoundingMode};
/// use writeable::Writeable;
///
/// let decimal = FixedDecimal::new_from_f64(0.012345678, DoublePrecision::Maximum).unwrap();
/// let decimal = FixedDecimal::try_from_f64(
/// -5.1,
/// DoublePrecision::Magnitude(-2, RoundingMode::Unnecessary)
/// )
/// .expect("Finite quantity with limited precision");
/// assert_eq!(decimal.write_to_string(), "-5.10");
///
/// let decimal = FixedDecimal::try_from_f64(
/// 0.012345678,
/// DoublePrecision::Floating
/// )
/// .expect("Finite quantity");
/// assert_eq!(decimal.write_to_string(), "0.012345678");
///
/// let decimal = FixedDecimal::new_from_f64(-123456.78, DoublePrecision::Maximum).unwrap();
/// assert_eq!(decimal.write_to_string(), "-123456.78");
///
/// let decimal = FixedDecimal::new_from_f64(12345678000., DoublePrecision::Maximum).unwrap();
/// let decimal = FixedDecimal::try_from_f64(
/// 12345678000.,
zbraniecki marked this conversation as resolved.
Show resolved Hide resolved
/// DoublePrecision::Integer
/// )
/// .expect("Finite, integer-valued quantity");
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: It would be useful to have the same number with different DoublePrecisions to compare the result.

If that's not feasible, than some description on when to use which (either here or by each variant of the enum), would help.

The way it reads now seems like I should chose my prevision variant depending on the value, rather than on the output.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion; I will implement this, but

The way it reads now seems like I should chose my prevision variant depending on the value, rather than on the output.

I argue that it is, in fact, dependent on the value. What you're doing is filling in "metadata" about the value that the f64 type is incapable of carrying internally.

Do you have suggestions on how to improve the docs to convey that subtlety?

Copy link
Member

Choose a reason for hiding this comment

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

Do you have suggestions on how to improve the docs to convey that subtlety?

I don't and maybe I'm misguided, but I think of myself as a perfect target audience for this docs.

I am an engineer, I have code that may have floats (example: user provides temperature in celsius) and I need to get plural category using it.
I don't have deep understanding of the problem space and I come to docs looking for TryFrom<f64>. Instead I find this new notion of DoublePrecision and the library authors ask me to pick a variant.

I don't know how and which one to use when. They can say "36.6 celsius","2.5 celsius", "25 celsius", "16.5 celsius" or "-10.2 celsius".

Doc string stating Specify that the floating point number is integer-valued. is confusing. I don't understand what does it mean for a float point number to be an integer-values. Will the float from the user be integer valued?

Next Magnitude - I can guess maybe that the doc string tells me that I can specify how many fration digits there will be in the output, but I'm not sure.
Next is SignificantDigits - how are significant digits different from magnitude? fraction digits?

Finally, Floating tells me that it'll have precision to maximum of IEEE - what's IEEE?

I may be overpainting here, but I think it's worth assuming the developer knows as little as I presented above.
Multiple examples by each variant showing what happens to several numbers under each variant would help visually distinguish the end result.

I think we're asking a lot from developers and forcing them to think in ways they often don't. Adding docs will be super valuable to educate them rather than frustrate.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am an engineer, I have code that may have floats (example: user provides temperature in celsius) and I need to get plural category using it.

I'm not totally aligned. An engineer in such a situation should have a NumberFormat, and they should be getting the plural form of the formatted number, not the input double.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not totally aligned. An engineer in such a situation should have a NumberFormat, and they should be getting the plural form of the formatted number, not the input double.

I see. I don't think we have a FormattedNumber type yet, so for now people will come with a f64.

Also, independently of my scenario, the docs should help someone understand the difference between variants and help them select the right one based on their needs in scenarios where they do not understand the nuances.

I think the current wording of the doc assumes quite detailed understanding of the details.

/// assert_eq!(decimal.write_to_string(), "12345678000");
/// ```
pub fn new_from_f64(float: f64, precision: DoublePrecision) -> Result<Self, Error> {
pub fn try_from_f64(float: f64, precision: DoublePrecision) -> Result<Self, Error> {
let mut decimal = Self::new_from_f64_raw(float)?;
let n_digits = decimal.digits.len();
// magnitude of the lowest digit in self.digits
Expand All @@ -932,7 +949,7 @@ impl FixedDecimal {
decimal.lower_magnitude = 0;
}
match precision {
DoublePrecision::Maximum => (),
DoublePrecision::Floating => (),
DoublePrecision::Integer => {
if lowest_magnitude < 0 {
return Err(Error::Limit);
Expand Down Expand Up @@ -989,6 +1006,7 @@ impl FixedDecimal {
decimal.check_invariants();
Ok(decimal)
}

/// Internal function for parsing directly from floats using ryū
fn new_from_f64_raw(float: f64) -> Result<Self, Error> {
if !float.is_finite() {
Expand Down Expand Up @@ -1165,12 +1183,12 @@ fn test_float() {
let cases = [
TestCase {
input: 1.234567,
precision: DoublePrecision::Maximum,
precision: DoublePrecision::Floating,
expected: "1.234567",
},
TestCase {
input: 888999.,
precision: DoublePrecision::Maximum,
precision: DoublePrecision::Floating,
expected: "888999",
},
// HalfExpand tests
Expand Down Expand Up @@ -1338,7 +1356,7 @@ fn test_float() {
];

for case in &cases {
let dec = FixedDecimal::new_from_f64(case.input, case.precision).unwrap();
let dec = FixedDecimal::try_from_f64(case.input, case.precision).unwrap();
writeable::assert_writeable_eq!(dec, case.expected, "{:?}", case);
}
}
Expand Down
1 change: 1 addition & 0 deletions utils/fixed_decimal/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ mod uint_iterator;
pub use decimal::DoublePrecision;

pub use decimal::FixedDecimal;
pub use decimal::RoundingMode;
use displaydoc::Display;
pub use signum::Signum;

Expand Down