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 division of two negative FastRational{BigInt} #25

Merged
merged 1 commit into from
Apr 10, 2020
Merged

Fix division of two negative FastRational{BigInt} #25

merged 1 commit into from
Apr 10, 2020

Conversation

Liozou
Copy link
Contributor

@Liozou Liozou commented Apr 10, 2020

Currently:

julia> FastQBig(-1) / FastQBig(-1)
ERROR: MethodError: no method matching typemax(::Type{BigInt})
Closest candidates are:
  typemax(::Type{LibGit2.Consts.GIT_MERGE_FILE_FAVOR}) at Enums.jl:191
  typemax(::Type{MbedTLS.CipherMode}) at Enums.jl:191
  typemax(::Type{Pkg.GitTools.GitMode}) at Enums.jl:191
  ...
Stacktrace:
 [1] /(::FastRational{BigInt}, ::FastRational{BigInt}) at /home/liozou/.julia/dev/FastRationals/src/generic.jl:120
 [2] top-level scope at none:1

as with any division of two negative FastRational{BigInt}, which hit me today as soon as I tried going from Rational{BigInt} to FastRational{BigInt}.

This PR fixes this by adding a specialized method for /(x::FastRational{T}, y::FastRational{T}) where {T<:BigInt} that simply bypasses the check with typemax(T). It's not a very clever fix, but I could not find another way to implement it that did not have any performance hit. In particular, I tried adding an isapplicable(typemax, T) check but T was not const-propagated (which is not a surprise I think), or, alternatively, hasmethod(typemax, Tuple{Type{BigInt}}) but it does not specialize on its arguments. If anyone has any idea, I will gladly update the PR! But in the meantime it does the job.

I added FastQBig among the tests (as well as the particular bug this PR fixes). Some tests that were commented out and marked with a # FIXME!!! actually work so I uncommented them. Let me know if this is a mistake!

@JeffreySarnoff
Copy link
Owner

good -- thank you

@JeffreySarnoff JeffreySarnoff merged commit 5a57d33 into JeffreySarnoff:master Apr 10, 2020
@JeffreySarnoff
Copy link
Owner

Please let me know how you use the package, what you do with it.

@JeffreySarnoff
Copy link
Owner

Your contribution is part of the current version of FastRationals.
After unpinning your development version and, optionally, removing FastRationals:
julia> using Pkg; Pkg.update() or, optionally,
julia> using Pkg; Pkg.add("FastRationals") should pull it in for you.

@Liozou
Copy link
Contributor Author

Liozou commented Apr 10, 2020

Wow, that was really fast, thanks!
I am working with periodic graphs and I need to compute a set of positions such that each vertex is at the average position of that of its neighbours. Since I actually want the mathematically exact solution, this amounts to solving a linear system with rational BigInts. Using FastRationals does give me some performance improvement on small systems, but it's slower on big systems (around 600 vertices per unit cell) most probably because the BigInts get prohibitively large during my LU decomposition. So I might not use the package eventually, or maybe only as an optimization once I figure for which case it does provide some speedup. But for the moment I have other issues to tackle first so I'll see about that later.

Anyway, glad I could help with that and thanks for the very fast merge!

@Liozou
Copy link
Contributor Author

Liozou commented Apr 12, 2020

I just now discovered the function Base.hastypemax which was exactly what I was looking for... So I guess the change can be rewritten nicely. But it working is probably more important anyway.

@JeffreySarnoff
Copy link
Owner

go ahead and PR that rewrite, please

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