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

Add trigonometric functions for special irrationals #21

Merged
merged 7 commits into from
Feb 26, 2023
Merged

Conversation

devmotion
Copy link
Member

Fixes #20.

This PR is a (largely) reduced and updated version of #14. Arguably it is quite uncontroversial since it just matches the existing behaviour of sin(pi) etc. in Base: JuliaLang/julia#42595

@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Base: 97.43% // Head: 98.24% // Increases project coverage by +0.80% 🎉

Coverage data is based on head (dd8b355) compared to base (6ecf42b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
+ Coverage   97.43%   98.24%   +0.80%     
==========================================
  Files           2        3       +1     
  Lines          39       57      +18     
==========================================
+ Hits           38       56      +18     
  Misses          1        1              
Impacted Files Coverage Δ
src/IrrationalConstants.jl 100.00% <ø> (ø)
src/trigonometric.jl 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@coveralls
Copy link

coveralls commented Feb 23, 2023

Pull Request Test Coverage Report for Build 4254120663

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 16 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 4223760967: 0.0%
Covered Lines: 53
Relevant Lines: 53

💛 - Coveralls

Copy link
Contributor

@hyrodium hyrodium left a comment

Choose a reason for hiding this comment

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

Note that sec(quartπ) does not have consistent accuracy because sec(x) is defined by inv(cos(x)).

julia> sec(quartπ)
1.414213562373095

julia> Float64(sqrt2)
1.4142135623730951

julia> inv(invsqrt2)  # same as inv(cos(invsqrt2))
1.414213562373095

This is one of the reasons why I would like to have sec(quartπ) === sqrt2 in #14.

test/runtests.jl Outdated Show resolved Hide resolved
@devmotion
Copy link
Member Author

This is one of the reasons why I would like to have sec(quartπ) === sqrt2 in #14.

I think we have to be consistent and always return Float64 (consistent with Base), otherwise it gets completely confusing. I improved the accuracy slightly by defining sec and csc for quartπ.

@hyrodium
Copy link
Contributor

I think we have to be consistent and always return Float64 (consistent with Base),

There is a counter example log(ℯ) === 1 (#14 (comment)).

otherwise it gets completely confusing.

I thought "return Irrational if we have the irrational, otherwise, return Float64" is the best approach (#14 (comment)), and I could not find that much confusion here.

However, I agree with merging this PR because this PR just improves the accuracy and there are no breaking changes. My last question is why my PR #14 is not merged yet. I updated the PR to return Float64 for all Irrational inputs as you suggested.

@devmotion
Copy link
Member Author

There is a counter example

The log definition is much older than the trigonometric definitions in Base and the odd one, it seems. I think when defining trigonometric functions for irrationals we should be consistent with how in Base trigonometric funtions for irrationals are implemented. And the reasoning there was quite clear, they should all return Float64.

My last question is why my PR #14 is not merged yet. I updated the PR to return Float64 for all Irrational inputs as you suggested.

It seems unfortunately I lost track of it, and with the recent changes it became outdated. Hence I decided to open a new PR. Generally, I also prefer smaller PRs with fewer changes (e.g., focused only on trigonometric functions) and without additional formatting changes.

@devmotion devmotion merged commit 12da953 into main Feb 26, 2023
@devmotion devmotion deleted the dw/triangular branch February 26, 2023 22:55
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.

Exact results for triangular functions
3 participants