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

Remove Grisu for Ryu; #29

Closed
wants to merge 2 commits into from
Closed

Conversation

isentropic
Copy link
Collaborator

@isentropic isentropic commented Jan 15, 2021

There have been a few modification in tests too, details explained in the review

Copy link
Collaborator Author

@isentropic isentropic left a comment

Choose a reason for hiding this comment

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

Currently passing all the test including the new one, except for engineering notation (for now)

@@ -36,8 +28,9 @@ end
@test Showoff.format_fixed_scientific(-Inf, 1, false) == "-∞"
@test Showoff.format_fixed_scientific(NaN, 1, false) == "NaN"
@test Showoff.format_fixed_scientific(0.012345678, 4, true) == "12.34568×10⁻³"
@test Showoff.format_fixed_scientific(0.012345678, 4, false) == "1.234568×10⁻²"
@test Showoff.format_fixed_scientific(-10.0, 4, false) == "-1.000×10¹"
@test Showoff.format_fixed_scientific(0.012345678, 4, false) == "1.2346×10⁻²"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test is modified as in the function docstring it says that (line 140 in the original code) the function should match @printf.

@@ -36,8 +28,9 @@ end
@test Showoff.format_fixed_scientific(-Inf, 1, false) == "-∞"
@test Showoff.format_fixed_scientific(NaN, 1, false) == "NaN"
@test Showoff.format_fixed_scientific(0.012345678, 4, true) == "12.34568×10⁻³"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test is failing, as I have still not implemented the engineering flag. Once the rest is approved, I will make sure this test passes, too.

@test Showoff.format_fixed_scientific(-10.0, 4, false) == "-1.000×10¹"
@test Showoff.format_fixed_scientific(0.012345678, 4, false) == "1.2346×10⁻²"
@test Showoff.format_fixed_scientific(-10.0, 4, false) == "-1.0000×10¹"
@test Showoff.format_fixed_scientific(-2.99999999999999956E-16, 2, false) == "-3.00×10⁻¹⁶"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a new test, the loingstanding issue #22

@timholy
Copy link
Member

timholy commented Jan 15, 2021

I just pushed a branch, teh/grisu, which has the work I started on this. Importantly, Grisu is only available on Julia 1.6 and higher, so unless you plan to drop support for earlier Julia versions we need to keep both implementations around.

@timholy
Copy link
Member

timholy commented Jan 15, 2021

I did mention the existence of that branch in #28 (comment)

@isentropic
Copy link
Collaborator Author

Right I just realized it. I'll see if I can get it to work. Surprisingly the code you wrote is almost the same I have here.

@timholy
Copy link
Member

timholy commented Jan 15, 2021

Indeed! The hard part is supporting the engineering notation; I tried to come up with a cheap trick, like multiplying or dividing by 10 to get it to the nearest multiple of 3, but then you end up doing carries on the Chars of the strings, and that's really ugly. So I quit working on it 😄

@isentropic isentropic closed this Feb 15, 2021
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.

2 participants