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

Improvements to FixedDecimal f64 APIs #1718

Merged
merged 2 commits into from
Mar 24, 2022
Merged

Conversation

sffc
Copy link
Member

@sffc sffc commented Mar 22, 2022

See #191 (maybe fixes it)

@sffc sffc requested review from zbraniecki and Manishearth March 22, 2022 01:51
@sffc sffc requested a review from a team as a code owner March 22, 2022 01:51
Copy link
Member

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

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

lgtm, but would like to ask for more details to help me select the right one for my use case or understand what considerations to take into account when choosing.

/// 12345678000.,
/// 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.

utils/fixed_decimal/src/decimal.rs Show resolved Hide resolved
@sffc
Copy link
Member Author

sffc commented Mar 22, 2022

lgtm, but would like to ask for more details to help me select the right one for my use case or understand what considerations to take into account when choosing.

Do the existing docs for DoublePrecision satisfy your request? I tried to make them more prominent by explicitly linking to them in the try_from_f64 docs.

@sffc
Copy link
Member Author

sffc commented Mar 24, 2022

I'm going to merge this despite the open comments, because we'll revisit these docs again in the context of #1267.

@sffc sffc merged commit d9756b8 into unicode-org:main Mar 24, 2022
@sffc sffc deleted the fixeddecimal-docs branch March 24, 2022 16:21
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