Expose slightly more API (members of d2s)
Closed this issue · 11 comments
I have a use case (crate) that benefits greatly from being able to use the function d2d in d2s.rs (and also f2d in f2s.rs). Since the modules d2s and f2s are marked private, I can't use these functions. Is there any way either:
- these 2 modules could be marked pub in main.rs? Or
- there could be a pub mod in lib.rs that pub uses the constants, FloatingDecimal types, d2d and f2d (these are already marked pub in their respective private modules)
This might require some other small documentation changes. In particular, it would probably be good to change the comments on FloatingDecimal32 to doc comments, and add similar comments in other spots. I could put together a PR with these changes if needed.
This would also require a minor version change.
I created PR #28 with an implementation of 2. It adds wrapper functions d2d and f2d in a module crate::decimal that would work, so the result is just exposing these 2 functions and the FloatingDecimal types
Closing per #28 (review).
Thanks for the response! To anyone in the future looking for this, I took the advice created the ryu-floating-decimal crate for this purpose:
https://crates.io/crates/ryu_floating_decimal
It would be very nice to have these functions in the main crate, rather than having to rely on a hard fork of the crate. Context: unicode-org/icu4x#724
Yeah, I'll just add: it seems better to consolidate this instead of having forks with different levels of maintenance. These APIs seem pretty useful.
@dtolnay - I'm not sure what is the best way to approach the request for reconsidering a position by the crate author.
In #28 you stated soft opposition to expanding the API and suggestion that the crate could be forked if needed.
In ICU4X we encountered a use case for the extension of the API and were able to produce measurable performance difference on what we consider to be potentially very hot path for Rust i18n layer: unicode-org/icu4x#724
The numbers are as follows:
from float (0.0012216734): string based - 56.873 ns, direct - 46.592 ns
from double (0.001221673434): string based - 94.091 ns, direct - 64.082 ns
20% and 32% performance improvement respectively is quite significant.
Additionally, we need the direct approach to correctly handle the post-decimal places parameter and scientific notation.
I recognize your right to refuse to extend the API and maintain the position that if we need the additional API we should fork the crate, but I would like to avoid that path if possible and in the light of the new findings ask you to reconsider.
Would you be open to reevaluation of that request?
Friendly ping @dtolnay - do you have thoughts on what @zbraniecki posted above?
Hi @sffc @Manishearth @zbraniecki, I took another look and I reaffirm that I do not want this in my library.
See https://github.com/ulfjack/ryu. It, and this library which is a line-for-line port of it, is for "converting floating point numbers to decimal strings". Upstream does not expose the thing you want. Upstream does not commit that producing a decimal string will always involve mantissa/exponent pair as an intermediate step, because their advertised purpose is only an algorithm for producing decimal strings. In the event that upstream's algorithm for producing a decimal string eliminates the intermediate step on some or all inputs, I do not have the expertise with the algorithm to continue supporting an augmented API while continuing to pull in their algorithm improvements, which is more important to me than your use case. Even if upstream were to add an API, their versioning philosophy is not necessarily the same as mine meaning it still wouldn't be a sufficient argument to mirror it here (with attention to the possibility that they drop that API in the future and this crate would need a new major version).
Insinuation that your work project is blocked from realizing an X% gain unless I maintain something additional on your behalf in my free time is unreasonable/inaccurate and not compelling to me. In my case I know how to respond to this, but when you communicate with other less experienced open source maintainers whose work you use, I would encourage you to think twice about how you apply this particular flavor of pressure to them.
In general all of my Rust libraries are calibrated to meet Y% of use cases for a consistent specific Y determined by my experience, which is less than 100. Chasing every niche use case (i.e. too high Y) destroys a library. The current ratio of users of ryu vs ryu_floating_decimal gives me no hesitation about the decision in this case.
What I would do in your shoes if you determine the performance of my code warrants it, is pull my repo as a submodule into a crate that you maintain, and use #[path = "..."] to grab whatever you need, and maintain the API that you need yourself.
Hi @dtolnay - thank you for reevaluating the request!
Insinuation that your work project is blocked from realizing an X% gain unless I maintain something additional on your behalf in my free time is unreasonable/inaccurate and not compelling to me. In my case I know how to respond to this, but when you communicate with other less experienced open source maintainers whose work you use, I would encourage you to think twice about how you apply this particular flavor of pressure to them.
I'm sorry you read our position this way, this was not our intent. As stated in my original comment, we felt that since your original position was made, we were able to produce new data that we wanted to offer for evaluation as a potential reason to change the decision.
You did that, and you concluded that you maintain your position, which is perfectly in line with our expectation and what I believe is healthy for project culture.
We do not expect you to maintain anything more than you want to maintain, and we believed that our input may allow you to improve the library.
We did not expect you to extend your library and maintain new codepaths just to supply our needs.
I'm sorry we failed to communicate that distinction better, and I'll take it as a piece of feedback for the future.
Thank you again for taking a look at this.
Thanks for the reply, and sorry for being pesky.
See https://github.com/ulfjack/ryu. It, and this library which is a line-for-line port of it, is for "converting floating point numbers to decimal strings". Upstream does not expose the thing you want.
In ryu/ryu_generic_128.h, it seems there are already functions with a very similar signature:
// A floating decimal representing (-1)^s * m * 10^e.
struct floating_decimal_128 {
__uint128_t mantissa;
int32_t exponent;
bool sign;
};
struct floating_decimal_128 float_to_fd128(float f);
struct floating_decimal_128 double_to_fd128(double d);Even if upstream were to add an API, their versioning philosophy is not necessarily the same as mine meaning it still wouldn't be a sufficient argument to mirror it here (with attention to the possibility that they drop that API in the future and this crate would need a new major version).
It seems this API has been public since early on in the project in 2018. I don't see a reason they would drop it.
Would you consider a pull request to your crate adding float_to_fd128 and double_to_fd128 with signatures mirroring upstream?