seq:add floating point support #6959
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sylvestre
reviewed
Dec 14, 2024
sylvestre
reviewed
Dec 14, 2024
sylvestre
reviewed
Dec 14, 2024
|
GNU testsuite comparison: |
bb32693 to
6ef9921
Compare
|
GNU testsuite comparison: |
b25be31 to
4f55794
Compare
|
GNU testsuite comparison: |
0fa1c61 to
5284545
Compare
|
GNU testsuite comparison: |
5284545 to
dc44647
Compare
|
GNU testsuite comparison: |
alexs-sh
commented
Dec 16, 2024
sylvestre
reviewed
Dec 17, 2024
dc44647 to
4d9a86e
Compare
|
GNU testsuite comparison: |
sylvestre
reviewed
Dec 18, 2024
gim913
reviewed
Dec 22, 2024
src/uu/seq/src/floatparse.rs
Outdated
| /// ``` | ||
| pub fn parse_hexadecimal_float(s: &str) -> Result<PreciseNumber, ParseNumberError> { | ||
| let value = parse_float(s)?; | ||
| let number = BigDecimal::from_f64(value).ok_or(ParseNumberError::Float)?; |
There was a problem hiding this comment.
in case of floating points, seq already supports arbitrary precision floating points, i.e.:
uu_seq 1e9865 1e9864 2e9865
the above limits hexadecimal support to f64, so p1023 is maximal binary exponent supported.
(i.e. uu_seq 0x1p1023 0x1p1023 will work, but uu_seq 0x1p1024 0x1p1024 will not)
(not sure if that is an issue or not, but imo it would be nice to have arbitrary precision supported in both variants)
There was a problem hiding this comment.
@gim913 Thanks, this is a good point. I also thought about it. And:
- I decided to start with native & simple f64 just to have something to demonstrate and discuss. :)
- arbitrary precision is a great option, but we should also consider compatibility with the original seq. From what I can see, GNU seq operates on
long double's with a15-bit exponent. This should serve as a bound for arbitrary precision. For example,
seq 0x1.4p16383 0
seq 0x1.4p16384 0
seq: invalid floating point argument: ‘0x1.4p16384’
Try 'seq --help' for more information.
4d9a86e to
a1f4acf
Compare
|
GNU testsuite comparison: |
2 similar comments
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
db975aa to
8b6317f
Compare
|
GNU testsuite comparison: |
29601c8 to
3b2664c
Compare
Turn on the float parser. Now it's possible to use hexadecimal floats as
parameters. For example,
cargo run -- 0x1p-1 3
0.5
1.5
2.5
Issue uutils#6935
Fix the test that fails after the implementation of the floating-point
format. The expected output now matches the GNU seq output.
seq 0xlmnop
seq: invalid floating point argument: ‘0xlmnop’
Issue uutils#6935
Let's update the structure and add separate functions for parsing each part of the number. It's simpler and allows us to maintain a consistent level of detail
This commit adds special cases for displaying floating-point values, making the behavior closer to the original GNU seq.
Add some tests from GNU coreutils that fail quite often. Feel free to add more.
Let's drop some latest modifications and start once again
Let's try to implement GNU-like precision detection. These are early steps to test the idea, and it will likely be modified and improved
Let's add and test a more GNU-like version of number precision.
Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>
Co-authored-by: Sylvestre Ledru <sylvestre@debian.org>
Keep the test cases for 2 and 3 arguments separately. Co-authored-by: Sylvestre Ledru <sylvestre@debian.org>
4792076 to
882da02
Compare
|
Looks great not blocking this PR but what would it take to support ? |
|
Hello @sylvestre
It seems that an exponent overflow triggers the error here. I need to review GNU seq, but I’m sure that replacing numbers with large negative exponents with zero values should work well. I’ll take a closer look at it in the next few days. Thank you for your observations |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
About
Hello,
This is the initial implementation of hexadecimal floating-point arguments in seq. It started as an attempt to add parsing for hexadecimal floats (issue #6935), but quickly grew into bigger changes.
Before
After
Expected
Changes
Added the
hexdecimalfloatmodule for parsing hex-floats and detecting the precision of numbers closer to GNU seq. I tried to keep the new functionality separate and minimize changes to the existing code to make changes more observable. I'm sure it’s possible unify number parsing to handle all types of values in one place. However, this seems like a big change and (IMHO) deserves a separate refactor-like PR.Changed the method for detecting and using precision for numbers. The initial approach of using the number of fractional digits from a
BigDecimaldidn’t match the GNU behavior and led to different representations. For now the precision is detected separately and this is not a part of thePreciseNumber(to avoid changing too much of the existing code).Added decimal/exponent notations for hexadecimal floats to match the
%Lgformat in GNU seq. Thesprintffunction fromuucoreis used for this.Added tests. During development, I found some additional mismatches with GNU seq. However, they are not directly related to hexadecimal floating points and could be addressed in future PRs.
Thank you