Add sqrt primitive.

[ummarikar]

This adds the sqrt primitive to yksom for Int, BigInt and Double.

[ummarikar]

I require some guidance. I have put the necessary tests for positive numbers but I am unsure of what to do with negative numbers. In Java SOM, sqrt of negative numbers returns "NaN" and I am not sure what the equivalent of that is in yksom after some initial searching in the codebase. Would appreciate some help!

[ltratt]

In Java SOM, sqrt of negative numbers returns "NaN" and I am not sure what the equivalent of that is in yksom after some initial searching in the codebase.

It should probably either produce NaN ("Not A Number") or throw an error. @smarr any thoughts on which? I'm inclined towards the latter, in part because sqrt in C throws an error (rather than producing NaN).

[ltratt]

OK, having taken a quick look at this, I think the correct thing to do (in IEEE 574) is to return NaN, so let's do that (which, conveniently, is what Rust's sqrt does anyway) and add some appropriate tests.

[ummarikar]

Just implemented NaN for negative square roots. Let me know if you want me to squash.

[ummarikar]

Removing all the above if statements produced the following error (for Integers):
thread 'main' panicked at 'the square root of a negative is imaginary', /Users/umarmarikar/.cargo/registry/src/github.com-1ecc6299db9ec823/num-integer-0.1.41/src/roots.rs:171:1 note: run with RUST_BACKTRACE=1 environment variable to display a backtrace.
However, the Double tests worked.
Should I keep the if statement in val.rs and integer.rs and remove it in double.rs?

[ltratt]

Let's remove the ifs that aren't needed, but we should definitely keep those that are!

[ummarikar]

Done!

[ltratt]

Please squash.

[ummarikar]

Squashed

[ltratt]

I just realised a problem. If self.val < 0 (for ints or big ints), we need to throw an error, not return a NaN. It's kinda weird if -1.sqrt() returns a double. We'll need tests for that too. We can make a new error (borrowing shamelessly from Python) VMError::DomainError or similar.

[ltratt]

If this is ready for rereview, you need to tell me explicitly.

[ummarikar]

If this is ready for rereview, you need to tell me explicitly.

I know, I only just got round to doing a double check now (I've had coursework from my modules recently), and I think there's nothing more I need to do so feel free to take a look!

[smarr]

I saw this before, and thought it is sensible.

However, triggered by SOM-st/SOM#24, does yksom give the expected results for 4 sqrt, 1 sqrt, and 3 sqrt?

---> 4 sqrt
it = 2
---> 1 sqrt
it = 1
---> 3 sqrt
it = 1.7320508075688772

and:

---> 4 sqrt class
it = Integer
---> 1 sqrt class
it = Integer
---> 3 sqrt class
it = Double

[ltratt]

Good point! No, this PR will not produce ints even when it could. I'm not even entirely sure I know the best way to implement this semantically. Obvious possibilities are 1) see if the FP number produced is int-like and if so convert it 2) convert the result to an int, square it, and see if you get the original answer again. I think those two routes will behave subtly different in some edge conditions, though I haven't checked. Any thoughts?

[smarr]

SOM and CSOM do basically the same for the case with an Integer as a receiver.

https://github.com/SOM-st/CSOM/blob/master/src/primitives/Integer.c#L267-L274

https://github.com/SOM-st/som-java/blob/3fab8ac7f34cedbbe15719fc607bceef25343d59/src/som/vmobjects/SInteger.java#L82-L89

Note that 4.0 sqrt class yields a Double.

[smarr]

Ah, and at this point, in the sense of a spec, I wouldn't consider this required behavior...

I think, I'd need to find the people who implemented that originally and get their reasoning, i.e., do some historical digging.

[ltratt]

https://github.com/SOM-st/CSOM/blob/master/src/primitives/Integer.c#L267-L274

OK, so that's option #1 of the two I suggested. It's a reasonable enough definition, at least from this PR's perspective. @ummarikar hopefully that gives you a bit of feedback for how to improve the PR a bit?

Ah, and at this point, in the sense of a spec, I wouldn't consider this required behavior...

Fair enough. I think it's not a bad idea to implement it similarly though -- if nothing else, reducing the points of divergence (as far as possible) to only clear bugs will probably make everybody's life easier!

[ummarikar]

https://github.com/SOM-st/CSOM/blob/master/src/primitives/Integer.c#L267-L274

OK, so that's option #1 of the two I suggested. It's a reasonable enough definition, at least from this PR's perspective. @ummarikar hopefully that gives you a bit of feedback for how to improve the PR a bit?

Ah, and at this point, in the sense of a spec, I wouldn't consider this required behavior...

Fair enough. I think it's not a bad idea to implement it similarly though -- if nothing else, reducing the points of divergence (as far as possible) to only clear bugs will probably make everybody's life easier!

Yes, this is great I'll implement it asap. Thanks for the feedback!

[ummarikar]

Just did some playing around with yksom. It has the following behaviour:

---> 4 sqrt class
it = Integer
---> 1 sqrt class
it = Integer
---> 3 sqrt class
it = Integer

I'll make sure that Integer sqrt produces a Double if the result is not a whole number.

[ltratt]

I'll make sure that Integer sqrt produces a Double if the result is not a whole number.

The links Stefan sent to other SOM's might give you a good pointer for how to go about this.

[ummarikar]

Added some code to val.rs which ensures that a Double is returned when the square root of an Integer is not a whole number. I was not sure if you want me to change the implementation of sqrt for BigInts when the result is not a whole number. The current implementation in Java SOM is to return an error in this case.

[ummarikar]

I also added an extra test case for "3 sqrt".

[ltratt]

We need to make sure both Ints and ArbInts have the same behaviour (from the SOM user's perspective they're one and the same).

[ltratt]

I think we also need checks along the lines of:

16 sqrt class println.

to make sure that things are printing Integer / Double as appropriate.

to check that

[ummarikar]

Sure I'll start that now.

[ummarikar]

All done.

[ltratt]

Please squash.

[ummarikar]

Squashed.

[ltratt]

bors r+

[bors]

Build succeeded

• buildbot/buildbot-build-script