-
Notifications
You must be signed in to change notification settings - Fork 36
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
DO NOT MERGE sqrt: add review comments. #35
Conversation
This is a dummy PR to simulate a code review on the square root code in the existing APD library.
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.
I've addressed these in #36
|
||
switch x.Sign() { | ||
case -1: | ||
res := InvalidOperation | ||
return c.goError(res) | ||
return c.goError(InvalidOperation) |
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.
Done on the nan
branch already.
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.
OK.
// variables in scope low. This refactoring doesn't quite work, but perhaps | ||
// you can refactor the calls below to use nc.Precision instead of keeping | ||
// workp in scope throughout. | ||
// TODO(mjibson): Also, what's nc? |
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.
nc is the "new context", which is used for the internal operations that require higher precision than the desired output. It is named consistently throughout many functions in apd.
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.
OK.
@@ -967,6 +997,9 @@ func (c *Context) Pow(d, x, y *Decimal) (Condition, error) { | |||
|
|||
// Quantize adjusts and rounds v as necessary so it is represented with | |||
// exponent exp and stores the result in d. | |||
// TODO(mjibson): "adjusts"? | |||
// TODO(mjibson): Could use a comment (not just here), or a wrapper Function | |||
// since you call this sometimes with the same argument for both d and v and that scares me. |
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 tests in the nan branch that verify you can safely call all functions where the result is also any operand and they still work correctly. Is that good enough?
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.
I'm not at all concerned about correctness because the testing throughout is ridiculously good, but it is weird to see, but that's fine. I'm OK with it.
@@ -399,7 +426,8 @@ func (c *Context) Sqrt(d, x *Decimal) (Condition, error) { | |||
// approx = 0.5 * (approx + f / approx) | |||
ed.Mul(approx, tmp, decimalHalf) | |||
} | |||
nc.Precision = workp + 1 | |||
// TODO(mjibson): why the plus 1? |
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.
Hey the tests pass without this. Removed.
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.
👍!
p := uint32(3) | ||
tmp := new(Decimal) | ||
|
||
// The algorithm in the paper says to use c.Precision + 2. decNumber uses | ||
// workp + 2. But we use workp + 5 to make the tests pass. This means it is | ||
// possible there are inputs we don't compute correctly and could be 1ulp off. | ||
for maxp := workp + 5; p != maxp; { | ||
for maxp := nc.Precision + 5; p != maxp; { | ||
// TODO(mjibson): why the minus 2? |
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.
Because that's what's written in the paper. Did I miss something?
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.
No, I missed it.
var f *Decimal | ||
// e is the number of digits in x. | ||
var e int64 | ||
// ed is ... |
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.
ed
is used consistently throughout as the error accumulator for internal operations. I think a comment describing that would be more annoying than useful since it's the same everywhere.
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.
OK.
workp = 7 | ||
|
||
// TODO(mjibson): Too many variables without comments/definitions. And nd is a temporary variable that goes out of scope, perhaps it can be closed off? I've given it a try but I'm not too happy with this either... Also, these are my *guesses* for what these variables represent. | ||
// f is ... |
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.
Both f
and e
are the same as in the paper. I don't think it makes sense to describe them here.
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.
OK. Nevertheless, I think the paper is too terse! Not blaming you, but I couldn't make sense of what those variables were from both the code and the paper. Still, you can leave it alone. We can't be rewriting papers as well.
if workp < 7 { | ||
workp = 7 | ||
|
||
// TODO(mjibson): Too many variables without comments/definitions. And nd is a temporary variable that goes out of scope, perhaps it can be closed off? I've given it a try but I'm not too happy with this either... Also, these are my *guesses* for what these variables represent. |
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.
nd
is the only variable I can see as benefiting from this, but it doesn't even gain that much because it requires a var e blah
above it, which I don't think is better. All the others are used throughout this function because that's how they work in the paper or decNumber.
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.
OK.
// TODO(mjibson): Also it would be good if we could keep the number of | ||
// variables in scope low. This refactoring doesn't quite work, but perhaps | ||
// you can refactor the calls below to use nc.Precision instead of keeping | ||
// workp in scope throughout. |
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.
I'm not sure how to do this without an extra variable because nc.Precision needs to be set to workp later down in the function, so we have to store it somewhere.
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.
OK
@@ -336,38 +336,62 @@ func (c *Context) Rem(d, x, y *Decimal) (Condition, error) { | |||
} | |||
|
|||
// Sqrt sets d to the square root of x. | |||
// Sqrt uses the Babylonian method for computing the square root, which uses |
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.
I really like this basic description of the algorithm and the big O for the number of steps. However I'm going to leave the reference as is to be consistent with the other references, none of which show up in the godoc. If you think it should be in the godoc, we can have that discussion and move them all at the same time.
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.
I'm okay with that. I didn't realize the change in position would have godoc effects.
Looks like everything is addressed, and we can continue the conversation at #36. |
This is a dummy PR to simulate a code review on the square root code
in the existing APD library.
Apologies that this is so late.
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)