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

fix negative-number engineering formatting #33

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

isentropic
Copy link
Collaborator

Tests passing now. The problem was with putting the floating point in case of negative numbers

@isentropic
Copy link
Collaborator Author

isentropic commented Feb 22, 2021

@timholy
Answering this, I think that now the bug with negative numbers for engineering notation is fixed.
Regarding the tests for the Grisu version of the code, is there a way to split the tests into two versions? Like it's done in the Showoff.jl main file, you've split the sources in case Ryu is present in the system to include ryu.jl file.

I have no idea how to split the test set gracefully for both Ryu and Grisu versions. The problem is that Grisu code was not without its issues hence these series of PRs. Perhaps it should just be declared obsolete and closed for further modification and testing. If new features to be added it should all go to ryu branch? In case of complaints from package mainteners, they could be asked to pin their dependency on the older Grisu only version of the code?

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.

Negative number fixes

@@ -36,7 +36,7 @@ end
@test Showoff.format_fixed_scientific(0.012345678, 4, true) == "12.346×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(-10.0, 4, false) == "-1.0000×10¹"
@test Showoff.format_fixed_scientific(-10.0, 4, true) == "-10.000×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.

It was same with line 38, thats why I just modified this line in accordance with the bug you found

@@ -103,11 +103,12 @@ function get_engineering_string(x::AbstractFloat, precision::Integer)
end

buf = IOBuffer()
negative_base_compensation = base_digits[1] == '-' ? 1 : 0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could not find a better solution to compensate for an extra index in case of negative numbers,

@timholy timholy merged commit 43e9505 into JuliaGraphics:teh/grisu Feb 22, 2021
@timholy
Copy link
Member

timholy commented Feb 22, 2021

Awesome. I agree with splitting the tests since, as you point out, some of the Grisu results just seem wrong (which makes it weird to enforce them with a test). Thanks for making this more self-consistent!

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.

None yet

2 participants