-
Notifications
You must be signed in to change notification settings - Fork 148
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
Added powc, powf, log and expf methods for complex numbers #137
Conversation
#[inline] | ||
pub fn powf(&self, value: T) -> Complex<T> { | ||
let (r, theta) = self.to_polar(); | ||
Complex::from_polar(&(r.powf(value)), &(theta*value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are unneeded parens around r.powf(value)
.
☔ The latest upstream changes (presumably #164) made this pull request unmergeable. Please resolve the merge conflicts. |
Thanks for your comment. I saw the merge failure, but there are also Am 4/13/2016 um 1:31 PM schrieb Homu:
|
Oh that's sad :(.
|
Removing pull request since of missing feedback and I don't want to keep this around as a dead entry any longer. |
It is not bad PR. Main reason why there was no feedback is that our work was focused on #164. Reopening. Just rebase it on top of current |
I really appreciate your feedback! Thanks! I'm working on your review comments and will update the pull request as soon as I'm done. |
#[inline] | ||
pub fn log(&self, base: T) -> Complex<T> { | ||
let (r, theta) = self.to_polar(); | ||
Complex::new(r.log(base), theta * T::one() / base.ln()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the one here, just theta / base.ln()
.
Please do write a formula comment for consistency on powf
and log
, as trivial as they may be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the one here, just
theta / base.ln()
.
There are times I feel stupid and sometimes this feeling is well justified. Thanks for the note I will update it soon!
Please update the PR title and message (first comment) as you go too, because these get immortalized in the merge commit. |
Oh and this is the resource I used to figure out the I double checked it by comparing some test cases against results obtained with GNU Octave. |
Added powc, powf, log and expf methods for complex numbers I would like to have a few functions added for complex numbers (powf, log and exp function with arbitrary base). I've provided an implementation with this commit. However it requires more work and discussion and I've added comments to point out the parts which I'm especially unhappy with. Would be nice to get some feedback so that we can improve this pull request first.
⚡ Test exempted - status |
I would like to have a few functions added for complex numbers (powf, log and exp function with arbitrary base). I've provided an implementation with this commit. However it requires more work and discussion and I've added comments to point out the parts which I'm especially unhappy with. Would be nice to get some feedback so that we can improve this pull request first.