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.

Sign up for GitHub

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

Jump to bottom

add sincospi #35816

Merged
merged 2 commits into from
Jun 23, 2020
Merged

add sincospi #35816

merged 2 commits into from
Jun 23, 2020

Conversation

simeonschaub
Copy link
Member

close #35799
ref #35792

simeonschaub
simeonschaub commented May 9, 2020
View reviewed changes
test/math.jl
Comment on lines +476 to +477
@test sinpi(complex(x, x)) ≈ Complex{Float64}(sinpi(complex(big(x), big(x))))
@test cospi(complex(x, x)) ≈ Complex{Float64}(cospi(complex(big(x), big(x))))
Copy link
Member Author

Choose a reason for hiding this comment

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

These should really be tested a bit more

@StefanKarpinski
Copy link
Member

This is great! I'm not qualified to review though. @simonbyrne, would you be willing?

@simonbyrne
Copy link
Contributor

Can we factor out the range reduction so that sinpi/cospi/sincospi all use the same underlying function?

@simeonschaub
Copy link
Member Author

I thought about this as well, but sinpi reduces to [-1,1], whereas cospi does [0,1], so they work a bit differently. One could perhaps use the same reduction for sinpi and sincospi, but I think factoring that out might add more complexity than is actually saved. Or do you think the deduplication is worth that complexity?

@simeonschaub simeonschaub mentioned this pull request May 12, 2020
Closed
@simeonschaub
Copy link
Member Author

simeonschaub commented May 25, 2020 •
edited
Loading

@simonbyrne What kind of refactoring did you have in mind?

@simeonschaub
Copy link
Member Author

Bump. What should be done here?

@simonbyrne simonbyrne closed this Jun 16, 2020
@simonbyrne simonbyrne reopened this Jun 16, 2020
@simeonschaub
Copy link
Member Author

Does this mean this is good to go as-is?

@simonbyrne
Copy link
Contributor

Yes, just wanted to figure out why the tests were failing.

@simonbyrne simonbyrne merged commit 6cd329c into JuliaLang:master Jun 23, 2020
@simonbyrne simonbyrne mentioned this pull request Jun 23, 2020
Closed
@simeonschaub simeonschaub mentioned this pull request Jun 23, 2020
Merged
mbauman added a commit to dlfivefifty/julia that referenced this pull request Jun 26, 2020
@mbauman
Merge remote-tracking branch 'origin/master' into patch-9
d7d91eb 
* origin/master: (232 commits)
  Add passthrough for non-Markdown docs (JuliaLang#36091)
  Fix pointer to no longer assume contiguity (JuliaLang#36405)
  Ensure string-hashing is defined before it gets used (JuliaLang#36411)
  Make compilecache atomic (JuliaLang#36416)
  add a test for JuliaLang#30739 (JuliaLang#36395)
  Fix broken links in docstring of `repeat` (JuliaLang#36376)
  fix and de-dup cached calls to `methods_by_ftype` in compiler (JuliaLang#36404)
  ml-matches: skip unnecessary work, when possible (JuliaLang#36413)
  gf: fix some issues with the move from using a tree to a hash lookup of leaf types (JuliaLang#36413)
  Add news and manual entry for sincospi (JuliaLang#36403)
  Check axes in Array(::AbstractArray) (fixes JuliaLang#36220) (JuliaLang#36397)
  add versions of `code_typed` and `which` that accept tuple types (JuliaLang#36389)
  Fix spelling of readdir. (JuliaLang#36409)
  add sincospi (JuliaLang#35816)
  fix showing methods with unicode gensymed variable names (JuliaLang#36396)
  Add doctest: eachslice (JuliaLang#36386)
  fix documentation typo ("Ingeger")
  Refactor `abstract_eval` to separate out statements and values (JuliaLang#36350)
  fix return type of `get!` on `IdDict` (JuliaLang#36383)
  Allow single option with REPL.TerminalMenus (JuliaLang#36369)
  ...
simeonschaub added a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
@simeonschaub
add sincospi (JuliaLang#35816)
8e4b22d
@simeonschaub simeonschaub mentioned this pull request Aug 14, 2020
Merged
@jmert jmert mentioned this pull request Oct 22, 2020
Merged
@ranocha ranocha mentioned this pull request Nov 2, 2020
Closed
@hyrodium hyrodium mentioned this pull request Feb 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonbyrne simonbyrne simonbyrne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add sincospi
3 participants
@simeonschaub @StefanKarpinski @simonbyrne