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 sin and cos #80

Merged
merged 3 commits into from
Jul 14, 2018
Merged

Add sin and cos #80

merged 3 commits into from
Jul 14, 2018

Conversation

porglezomp
Copy link
Contributor

@porglezomp porglezomp commented Jul 14, 2018 •
edited
Loading

Closes #11
Closes #33

@porglezomp porglezomp changed the title [WIP] Add sin and cos Add sin and cos Jul 14, 2018
@porglezomp porglezomp mentioned this pull request Jul 14, 2018
Closed
@porglezomp porglezomp force-pushed the sin-cos branch 3 times, most recently from 7011ad4 to 1fa5b83 Compare July 14, 2018 19:36
japaric
japaric suggested changes Jul 14, 2018
View reviewed changes
Copy link
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

LGTM except for the removal of the re-exports.

src/math/mod.rs Outdated
mod rem_pio2_large;
mod rem_pio2f;

use self::{k_cosf::k_cosf, k_sinf::k_sinf, rem_pio2_large::rem_pio2_large, rem_pio2f::rem_pio2f};
Copy link
Member

Choose a reason for hiding this comment

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

These re-export are OK to keep as they are private and won't become part of this crate public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I ended up removing that really because it was the single line and I hadn't needed it, I'll change things up so that it's multiple lines.

@japaric
Copy link
Member

japaric commented Jul 14, 2018

Thanks @porglezomp

bors r+

bors bot added a commit that referenced this pull request Jul 14, 2018
@bors @porglezomp
Merge #80
1ddc606 
80: Add sin and cos r=japaric a=porglezomp

Closes #11 
Closes #33

Co-authored-by: C Jones <code@calebjones.net>
@bors
Copy link
Contributor

bors bot commented Jul 14, 2018

Build failed

japaric
japaric reviewed Jul 14, 2018
View reviewed changes
src/math/rem_pio2.rs Outdated
return (n, ty[0], ty[1]);
}

fn rem_pio2_large(_tx: &[f64], _ty: &mut [f64; 3], _e0: i32, _prec: i32) -> i32 {
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be replaced with an import, I think

@japaric
Copy link
Member

japaric commented Jul 14, 2018

The cos implementation has test failures:

thread 'cos' panicked at 'input: 17147092039258976684, output: 4603354832323624468, expected: 13827245563309741773', tests/cos.rs:10021:17

thread 'cos' panicked at 'input: 6621462121365111395, output: 4605518424732097566, expected: 13828388675460851964', tests/cos.rs:10021:17

Note that the literals are integer representations of floats and need to be converted to floats using f64::from_bits.

@porglezomp
Copy link
Contributor Author

I think I fixed those issues, the x1p24 constant was wrong. Any idea why CI is currently not running on this PR?

@japaric
Copy link
Member

japaric commented Jul 14, 2018 via email

@japaric
Copy link
Member

japaric commented Jul 14, 2018

bors try

@japaric
Copy link
Member

japaric commented Jul 14, 2018

bors r+

bors bot added a commit that referenced this pull request Jul 14, 2018
@bors @porglezomp
Merge #80
4c0992b 
80: Add sin and cos r=japaric a=porglezomp

Closes #11 
Closes #33

Co-authored-by: C Jones <code@calebjones.net>
@bors
Copy link
Contributor

bors bot commented Jul 14, 2018

Build succeeded

@bors bors bot merged commit 4fb6e33 into rust-lang:master Jul 14, 2018
@porglezomp porglezomp deleted the sin-cos branch July 14, 2018 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@japaric japaric japaric 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.
2 participants
@porglezomp @japaric