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

rationalize method for integer #43564

Closed
ErikQQY opened this issue Dec 27, 2021 · 8 comments
Closed

rationalize method for integer #43564

ErikQQY opened this issue Dec 27, 2021 · 8 comments

Comments

@ErikQQY
Copy link

ErikQQY commented Dec 27, 2021

Hello guys, I notice the rationalize method is only supporting floating point numbers, and support for complex floating point numbers are in process, I think maybe this method should also support rationalizing integer.

In Matlab, there are rats method to get rationalized expression of any number, floating point numbers, complex numbers and integer.

And in python, there is also integer numbers rationalizing method.

So I think the rationalize(x::Integer; tol::Real) should be included in this method.

Would be happy to hear your thoughts.

@vtjnash
Copy link
Member

vtjnash commented Dec 27, 2021

The main challenge is simply that it is defined to be an approximation from a decimal number, while integer conversion is always exact:

julia> Rational(10)
10//1

julia> Rational(.1)
3602879701896397//36028797018963968

julia> rationalize(.1)
1//10

help?> rationalize
search: rationalize Rational Irrational AbstractIrrational

  rationalize([T<:Integer=Int,] x; tol::Real=eps(x))

  Approximate floating point number x as a Rational number with components of the given integer type. The result will differ from x by no more than
  tol.

@ErikQQY
Copy link
Author

ErikQQY commented Dec 28, 2021

Yes, I see the Rational(::Integer) could return the rational expression of integer, but I think when users are using rationalize to handle their data, it is a little weird that rationalize can’t rationalize integer🤨.

I don’t think this method should only limit to approximation of decimal number, a rational number should be a number that can be expressed as the quotient of two integers, so it is reasonable to make the rationalize able to handle integer.

BTW, from my point, I would use the rationalize method to get rationalized expression of an array, there is also no such method any way.🤔

julia> arr = [1.2, 2, 3.4]
julia> rationalize(arr)
Error……

@dkarrasch
Copy link
Member

BTW, from my point, I would use the rationalize method to get rationalized expression of an array, there is also no such method any way

You would use broadcasting for this:

julia> arr = [1.2, 2, 3.4]
3-element Vector{Float64}:
 1.2
 2.0
 3.4

julia> rationalize.(arr)
3-element Vector{Rational{Int64}}:
  6//5
  2//1
 17//5

@ErikQQY
Copy link
Author

ErikQQY commented Dec 29, 2021

@dkarrasch Yes, the broadcasting is working, but in your example, the integer 2 is first being converted to a floating number

Then the rationalize method works on 2.0

I think adding the rationalize method for integers would be more suitable for more data types.

@dkarrasch
Copy link
Member

But that has nothing to do with the rationalize function. As you can see from the example, that promotion takes place before rationalize. If you want to avoid that, you'd need to write it as a tuple.

@ErikQQY
Copy link
Author

ErikQQY commented Dec 29, 2021

Thanks for your immediate response!

Yes, the type promotion happened after we created the array, the problem is, for example, if I pass an integer to rationalize:

julia> rationalize(1)
ERROR:...

Or, if I pass an integer array to rationalize.():

julia> rationalize.([1, 2, 3])
ERROR:...

@dkarrasch
Copy link
Member

Yes, but all of this has nothing to do with a missing method of rationalize for arrays as you seemed to suggest initially. There is only one issue here: rationalize for integers, which is dealt with IIUC in #43427 alongside.

@ErikQQY
Copy link
Author

ErikQQY commented Dec 29, 2021

Wow thanks! Didn’t notice it at first

That PR is adding the relating method~

So I would close this issue😄

@ErikQQY ErikQQY closed this as completed Dec 29, 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

No branches or pull requests

3 participants