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 trigonometric functions to not raise on f32 inputs #1051

Merged

Conversation

sasikumar87
Copy link
Contributor

Fixes #1047

Copy link
Contributor

@billylanchantin billylanchantin left a comment

Choose a reason for hiding this comment

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

Approved pending the precision issues with the tan test.

It looked like they were off by a single decimal place 🙄. Let us know if they persist.

@josevalim
Copy link
Member

Precision issues will happen on different machines, OSes and architectures. So one option is to compare that they are within certain delta.

@billylanchantin
Copy link
Contributor

@josevalim Do we have a helper somewhere that acts like Nx.all_close? That'd be handy here.

@josevalim
Copy link
Member

We don't but it should be easy to implement. Subtract two series, compute the absolute, see if they are within delta and return true if all true.

@billylanchantin
Copy link
Contributor

billylanchantin commented Jan 9, 2025

Agreed. It'd be generally helpful too.

pi = :math.pi()
radians_list = [-2 * pi, -pi, -pi / 2, 0, pi / 2, pi, 2 * pi]
degrees_list = [-360.0, -180.0, -90.0, 0.0, 90.0, 180.0, 360.0]

radians = Series.from_list(radians_list, dtype: :f32)
degrees = Series.from_list(degrees_list, dtype: :f32)

result = Series.degrees(radians)
assert all_close?(result, degrees)
assert Series.dtype(result) == {:f, 64}

# ...

defp all_close?(%Series{} = a, %Series{} = b, tol \\ 1.0e-8) do
  Series.subtract(a, b)
  |> Series.abs()
  |> Series.less_equal(tol)
  |> Series.all?()
end

@sasikumar87
Copy link
Contributor Author

@billylanchantin fixed the test.

@billylanchantin billylanchantin merged commit 672f111 into elixir-explorer:main Jan 10, 2025
4 checks passed
@billylanchantin
Copy link
Contributor

Thanks, @sasikumar87!

@sasikumar87 sasikumar87 deleted the fix-trig-functions-f32-type branch January 10, 2025 14:56
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.

Trig functions raise on f32 dtypes
3 participants