Implement Base.parse for Rational

[henriquebecker91]

Sparked from #18328, minimal change to get the PR started.

Design decisions that should be conscious and I need you people to do:

1. Should parse(Rational{Int8}, "10") give 10//1? This is, parse single integers as Rationals with denominator 1.
2. Should parse(Rational{Int}, "1/2") or parse(Rational{Int}, "1///2") give 1//2, this is, will we be strict about the number of slashes?
3. Should parse(Rational, "10") (i.e., the UnionAll type) choose a default (or the "most adequate") concrete subtype and just work? Or this can be left to another PR?

[oscardssmith]

I would say

1. yes
2. 1 or 2 / allowed, more than 2 aren't
3. default to Rational{Int} for Rational, but don't support other abstract types. This can also be left to another PR.

[rfourquet]

I'm not sure for 3, as parse(Integer, "10") errors and doesn't pick a "default".

[henriquebecker91]

I kinda agree with @rfourquet on 3.

4. Should I throw better error messages (instead of letting it fail) for some cases?
   1. Something like 1/2/... should have a custom message or let it fail trying to parse 2/... as some integer?
   2. If there is not a / or // just let it fail trying to parse the string as some integer?
   3. Anything else you people think it merits a better error message?

[henriquebecker91]

Improved the code, unless someone believe it should work differently, or support more features, the code appears to be done. How is the documentation/testing policy for this kind of function? (i.e., a specialization of parse?)

[Liozou]

A few style and performance improvements.

I think tests would be good around https://github.com/Liozou/julia/blob/b63ae3b88a838b050aca60c819557d80a4b4ca48/test/rational.jl#L268 since that's where show, read and write are tested for Rational.

[henriquebecker91]

I have already created a long testset (that will probably be cut short after I commit it) but before I commit it, this is one strange corner case I found, is this behavior correct/expected?

julia> parse(Rational{Int}, "100 10")
ERROR: ArgumentError: extra characters after whitespace in "100 10"
Stacktrace:
 [1] tryparse_internal(::Type{Int64}, ::String, ::Int64, ::Int64, ::Int64, ::Bool) at ./parse.jl:169
 [2] parse(::Type{Int64}, ::String; base::Nothing) at ./parse.jl:238
 [3] parse at ./parse.jl:238 [inlined]
 [4] parse(::Type{Rational{Int64}}, ::String) at ./REPL[2]:4
 [5] top-level scope at REPL[7]:1

julia> parse(Rational{BigInt}, "100 10")
10010//1

[Liozou]

Comes from

julia> parse(Int, "100 10")
ERROR: ArgumentError: extra characters after whitespace in "100 10"

julia> parse(BigInt, "100 10")
10010

The second case is the oddity here, and it seems to originate from

julia/base/gmp.jl

Line 284 in f5d1557

err = GC.@preserve bstr MPZ.set_str!(z, pointer(bstr)+(i-firstindex(bstr)), base)

aka MPZ.set_str! does not follow Julia conventions for parsing numbers.
In any case, I feel like this is orthogonal to your implementation: whether it deserves to be fixed should be another issue.

[henriquebecker91]

Ok. I will exclude the test case for this kinda of whitespace.

[Liozou]

I opened an issue (#44570) to avoid losing track of that

[henriquebecker91]

Tests added, they are very comprehensive, so I understand if they need to be cut down.

[henriquebecker91]

It seems like the mention was a mistake but let me use this opportunity to ask if there is something I should be doing, or this is already outside my hands?

[Liozou]

Thanks for the reminder! The testset is indeed quite extensive but I think it can be reduced like this:

• Only keep one of the tested types (Int for simplicity) for the test_throws.
• Drop Int8 and UInt8 since there is nothing specific to them (or am I missing something?).
• For better readability, factor the common tests by doing something like:

   for T in (Int, BigInt)
      @test parse(Rational{T}, string(T(-10)) == T(-10) // T(1)
      ...
   end

In addition, I think it would be nice to check that parsing errors when trying to fit a number which is too big for a type, say parse(Rational{Int8}, "720//13") and does not silently output garbage. I also wonder whether we should allow parse(Rational{Int8}, "720//30") (which is actually 24//1, so can be represented with Int8) but I don't think it's necessary, I don't expect there to be many rationals written in a reducible form anyway.

Could you also add a single test for "123 // -456" for example, to check that the resulting rational is well-formed (with the sign on the numerator)? And maybe also one last for mixed arguments type, like "0xab // 123" (quite uncommon but might as well test it if it works).

The Rational{Bool} test is fine. Regarding the previous issue, I'd suggest adding the following snippet

try  # issue 44570
   parse(Rational{BigInt}, "100 10")
   @test_broken false
catch
   @test_broken true
end

which will result in a @test_broken until #44570 is fixed.

[henriquebecker91]

Drop Int8 and UInt8 since there is nothing specific to them (or am I missing something?).

Their literals are different, for example, 0x0a.

For better readability, factor the common tests by doing something like:

I started like this, and then I had the BigInt error and discovered that is impossible to find the exact set of conditions that triggered the error unless they are exhaustively written out. The test error message does not give what is T in the loop. I have the factored code already in hands, as I use it with printlns to actually generate the code I committed. But maybe it is better to reduce the number of distinct cases and keep it without a loop.

I think it would be nice to check that parsing errors when trying to fit a number which is too big for a type, say parse(Rational{Int8}, "720//13")

Will do. Just for the record as we parse numerator and denominator as T the user will be getting an OverflowError.

I also wonder whether we should allow parse(Rational{Int8}, "720//30") (which is actually 24//1, so can be represented with Int8) but I don't think it's necessary, I don't expect there to be many rationals written in a reducible form anyway.

I also agree that the smart thing here is to not try to be overly clever. We would need to read all numbers as BigInt and then test if (after gcd dividsion) they can fit just to support the uncommon case (that could be fixed on the other side) of not writing Rationals in canonical form (and if the data comes from Julia this will very rarely happen).

Could you also add a single test for "123 // -456" for example, to check that the resulting rational is well-formed (with the sign on the numerator)?

This currently parses as -41//152 should it not? Should we go out of the way to throw an error if the denominator is valid but recognized as a negative number? I need to change the code if this is the case.

And maybe also one last for mixed arguments type, like "0xab // 123" (quite uncommon but might as well test it if it works).
The Rational{Bool} test is fine. Regarding the previous issue, I'd suggest adding the following snippet

Will do both.

[Liozou]

Drop Int8 and UInt8 since there is nothing specific to them (or am I missing something?).

Their literals are different, for example, 0x0a.

Ah yes you're right, I had indeed missed that!
After thinking a bit more regarding this and the BigInt thing, I believe it's better to only keep the Int tests as they are, and then add just a few individual tests for BigInt, UInt and UInt8 for example. The goal is not to duplicate the tests from test/parse.jl which already cover the parse(<:Integer, ...) methods, so we should focus on tests for parsing the fraction bar which the Int tests already cover by themselves.

(to be honest, I don't feel like an authority on testing really, feel free to adapt if you believe some additional tests are worth it)

Just for the record as we parse numerator and denominator as T the user will be getting an OverflowError

I think that makes sense so it's good.

This currently parses as -41//152 should it not?

-41//152 is perfect, I just want to check that it is not 41//-152 or something like that (which could be created by an improper use of unsafe_rational).

[henriquebecker91]

Tests reduced and improved, all passing but the BigInt one that is marked as broken.

[henriquebecker91]

Last polishing of the test code done.

[henriquebecker91]

There is something for me to do yet?

[oscardssmith]

Can you rebase this branch? It would be good to get CI passing on mac. Overall, I think this looks good.

[henriquebecker91]

I never know if I did this part of the process correctly. I followed this tutorial (https://akrabat.com/the-beginners-guide-to-rebasing-your-pr/) but did not find any conflicts (and therefore skipped the conflict resolution step). Can you check if it is everything ok?

[henriquebecker91]

I am not sure if the builds are failing because my changes, how do I check?

[Liozou]

LGTM.
The rebase looks OK. Regarding the CI failures occurring here, they also appear on other unrelated PRs so it all looks good to me.

[henriquebecker91]

Some reason this was not merged yet?

[oscardssmith]

does I forgot count as aa good reason?

[henriquebecker91]

The best one. It means there is nothing wrong with the PR.