Take sign into account when rounding numbers in (-1, 1), fixes #136

[danilopedraza]

I think this should be enough to fix the problem. Let me know if I should make a change.

[akubera]

I'm sorry I never commented on this.

BigDecimal::round was the old implementation before Context and RoundingMode were added. I think the better option here is to replace the implementation of round with a call to BigDecimal::with_scale_round, using the default rounding mode.

    pub fn round(&self, round_digits: i64) -> BigDecimal {
        self.with_scale_round(round_digits, Context::default().rounding_mode())
    }

Doing that correctly rounds these test cases:

            ("-1.5", 0, "-2"),
            ("-1.2", 0, "-1"),
            ("-0.68", 0, "-1"),
            ("-0.5", 0, "0"),
            ("-0.49", 0, "0"),

[akubera]

Your commit has highlighted something something to me:

BigDecimal::new(BigInt::from_biguint(sign, BigUint::one()), round_digits);

There should be an easier way to copy the sign of a value.

BigDecimal::from_copysign(1, sign).with_scale(round_digits);

/// Create bigdecimal from `value`, copying the given sign (if not zero) 
pub fn<T: Into<BigDecimal>> from_copysign(value: T, sign: Sign) -> Self {
    let BigDecimal { scale, int_val } = value.into();
    let ( orig_sign, uint_val ) = int_val.into_parts();
    // NoSign means value is zero, and should remain so
    let new_sign = if orig_sign == Sign::NoSign { orig_sign } else { sign };

    // result Big
    BigDecimal { scale, int_val: BigInt::from_biguint(new_sign, uint_val) }
}

Does that seem good?

[akubera]

or from_value_copying_sign?

from_copysign sounds like it would be constructing from a "copysign" object.

from_value_and_sign might be best. We'll leave "copying" to mean taking the sign from another value, not a sign itself.

let a = x.with_sign(Plus);
let a = x.with_copied_sign(Plus);

fn with_sign(&self, sign: Sign) -> BigDecimal;

// maybe "change" as the mutation verb? "set_sign"?
fn change_sign(&mut self, sign: Sign);

// copies the sign of any signed object  (note u8/16/etc are NOT signed)
fn copy_sign<S: num_traits::Signed>(&mut self, signed: S);

Open for opinions on verbage. What did you look for first when you went to create a "±1" ?

[danilopedraza]

BigDecimal::round was the old implementation before Context and RoundingMode were added. I think the better option here is to replace the implementation of round with a call to BigDecimal::with_scale_round, using the default rounding mode.

Haha, I suspected this. I can do this change.

from_copysign sounds like it would be constructing from a "copysign" object.

Yeah, I thought that too. from_value_and_sign sounds like a good name to me. It may be a bit long, but in the end is a kinda long operation.

Open for opinions on verbage. What did you look for first when you went to create a "±1" ?

I was aware of the from_biguint function from previous use of the BigInt library. I don't have better ideas than the ones you already showed. My favorite is with_sign, since looks nice for composition. change_sign is my second choice, since it would be convenient to avoid copying (is that really a concern?).

[akubera]

Sorry, I've been making changes, rebase onto new trunk and push and the ci will pass again

[danilopedraza]

Haha, that was a bit clumsy. I think it's fine now.

[akubera]

Alright, merged. Thanks

[akubera]

That's what I like to see.