Skip to content

Conversation

KristofferC
Copy link
Contributor

This fixes the usage of Base.Grisu on the upcoming 1.6 where this is removed.

Also, it fixes a few tests that test different printing.
Tests written like this are quite sensitive to small printing tweaks made to e.g. types in Julia.
In the tests I fixed, I tried to show that it is better to interpolate the whole type
in the string so that Julia will use its own printing for it instead of manually copy pasting
the type printing for a single Julia version.

After fixing one file, there was a lot of similar test errors in other files but at least
this gets it a bit on the way.

@odow
Copy link
Member

odow commented Nov 11, 2020

there was a lot of similar test errors in other files but at least
this gets it a bit on the way.

Thanks! I'll take it from here.

@odow
Copy link
Member

odow commented Nov 12, 2020

Tests will fail due to some issues in MutableArithmetics.

@KristofferC
Copy link
Contributor Author

Another option instead of using Grisu is to transition to the new Rye floating point printer that is in Julia Base:

https://github.com/JuliaLang/julia/blob/93fb785831dcfcc442f82fab8746f0244c5274ae/base/ryu/Ryu.jl#L19-L51

Of course, even better would be if it is possible to do this whole thing without using Julia internals. I haven't looked into it enough to know if such high level of control is needed that calling e.g. the internal Grisu is a requirement.

@odow
Copy link
Member

odow commented Nov 12, 2020

It seems like we used Grisu in due to some issues with Julia printing floats via @printf in 2014, and it stayed around through various refactoring: jump-dev/JuMP.jl#102, jump-dev/JuMP.jl#191. Base.print seems to work fine for floats now.

@odow
Copy link
Member

odow commented Nov 19, 2020

Tests passing on 1.6.

@KristofferC
Copy link
Contributor Author

Would be nice with a new release with this whenever possible.

@blegat blegat added this to the v0.9.19 milestone Dec 1, 2020
@blegat
Copy link
Member

blegat commented Dec 1, 2020

Let's do it: #1204

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants