dnsl48 / fraction

[Rust] Lossless fractions and decimals; drop-in float replacement
Apache License 2.0
83 stars 25 forks source link

Weird huge values when getting the denominator of a fraction #64

Closed sciguyryan closed 2 years ago

sciguyryan commented 2 years ago

Hi.

I can into this while working on a chemical equation balancer, this may be a bug, or it might be something I'm going wrong.

I've gotten all of the matrix stuff done, and it yields a set of coefficients. The ones in my example are as follows: [-0.5, -0.33333334, -0.16666667, 1.0].

I create a fraction from these numbers, as follows:

let f = Fraction::from(*coeff);
if let Some(d) = f.denom() {
    denoms.push(*d as f32);
} else {
    panic!("Unable to obtain a valid denominator.");
}

Which should be correct, I believe.

The code produces a list of denominators as follows: [2.0, 50000000.0, 100000000.0, 1.0]

The first and last entries look fine, but there definitely appears to be something wrong with the other two entries. If this isn't a bug, then what am I doing wrong here and how can I fix it?

Thanks in advance.

dnsl48 commented 2 years ago

Hi @sciguyryan,

I am not too sure I am following exactly what you are trying to achieve, but I think it can result from the fraction optimization through LCD (lowest common denominator). If you could post the numerators, it could give us more visibility about what exactly is happening with the data.

Cheers

sciguyryan commented 2 years ago

Hi @sciguyryan,

I am not too sure I am following exactly what you are trying to achieve, but I think it can result from the fraction optimization through LCD (lowest common denominator). If you could post the numerators, it could give us more visibility about what exactly is happening with the data.

Cheers

Sure, that's not a problem. Sorry for the delay in my reply here, uni work has been hectic recently.

inputs = [-0.5, -0.33333334, -0.16666667, 1.0] numerators = [1.0, 16666667.0, 16666667.0, 1.0] denominators = [2.0, 50000000.0, 100000000.0, 1.0]

This is run through the following code:

    let mut denoms = Vec::with_capacity(len);
    let mut noms = Vec::with_capacity(len);
    for coeff in &coeffs {
        let f = Fraction::from(*coeff);
        if let Some(d) = f.denom() {
            denoms.push(*d as f32);
        } else {
            panic!("Unable to obtain denominator.");
        }

        if let Some(n) = f.numer() {
            noms.push(*n as f32);
        }
        else {
            panic!("Unable to obtain numerator.");
        }
    }

Basically the same as above, but with the numerator also included.

dnsl48 commented 2 years ago

Sorry for the delay in my reply here, uni work has been hectic recently.

Haha, you're too polite, given it took me 5 days to respond initially. No worries, sometimes it takes months to get a response in some repositories. We are all busy people :)

the following code

Ok, so the data appears to be correct to me. 16666667/50000000 = 0.33333334. What would you expect to get as denominators instead of this?

My guess is you may want to be working with fractions instead of decimals in there in the first place, then you would get 1/3 instead of 0.33333333333. Otherwise, when you get a fraction representation of the decimal (0.33333334) you end up with the fraction representing that exact decimal.

sciguyryan commented 2 years ago

Haha, you're too polite, given it took me 5 days to respond initially. No worries, sometimes it takes months to get a response in some repositories. We are all busy people :)

I work on software myself, I know what it's like to be busy. We do this as a hobby, no need to worry about a delayed response!

My guess is you may want to be working with fractions instead of decimals in there in the first place, then you would get 1/3 instead of 0.33333333333. Otherwise, when you get a fraction representation of the decimal (0.33333334) you end up with the fraction representing that exact decimal.

Yes, that is exactly correct. Essentially I need to get the denominators of the coefficients (represented as a fraction), calculate the LCM of those denominators to calculate the multiple to be applied to the matrix, which will give the coefficient for each part of the equation. I thought that is what I was doing with my code above, but if that isn't correct then do you have any pointers as to what I'm doing wrong here and how to do this correctly?

I would expect the fractions to be: [1/2, 1/3, 1/6, 1/1], yielding denominators of: [2, 3, 6, 1]. That would then yield a LCM of 6, which is what I would expect.

The last time I wrote one of these was using JavaScript (many years ago), and didn't run into these problems. Then again... JavaScript is far less strict when it comes to handling data types.

dnsl48 commented 2 years ago

Ok, in that case you may need to update your coeffs list so it contains fractions, not floats/decimals. Right now, its initialization is outside of your example, but apparently it contains a bunch of floats.

That means you may end up with something like this:

   // the list of fractions - [1/2, 1/3, 1/6, 1/1]
   let coeffs = vec![
      Fraction::new(1u8, 2u8),
      Fraction::new(1u8, 3u8),
      Fraction::new(1u8, 6u8),
      Fraction::new(1u8, 1u8),
   ];

    let mut denoms = Vec::with_capacity(len);
    let mut noms = Vec::with_capacity(len);

    for f in &coeffs {
        if let Some(d) = f.denom() {
            denoms.push(*d as f32);
        } else {
            panic!("Unable to obtain denominator.");
        }

        if let Some(n) = f.numer() {
            noms.push(*n as f32);
        } else {
            panic!("Unable to obtain numerator.");
        }
    }
sciguyryan commented 2 years ago

Would that not have exactly the same problem when I attempt to extract the denominators later? Since I don't know the coefficients ahead of time (they are calculated via RRFE matrix-reduction on a per-equation basis) they have to be initialized from a f32.

With that in mind, if I store the Fraction structs, I'll still run into exactly the same problem when it comes to extracting the denominator for the LCM calculation later.

dnsl48 commented 2 years ago

I don't know the coefficients ahead of time (they are calculated via RRFE matrix-reduction on a per-equation basis) they have to be initialized from a f32

Well, this is the tricky part. Fraction is a lossless type. f32 is a lossy type. We cannot represent 1/3 by a float (although we can by a fraction). Thus, when you convert a f32 value into a Fraction value, you end up not with 1/3, but with 0.33333334/100000000, which is optimized via LCD and becomes 16666667/50000000. The fraction library cannot guess about the lost data and convert 0.3 or 0.33 to 1/3, because for the fraction those look like 3/10 and 33/100.

sciguyryan commented 2 years ago

I don't know the coefficients ahead of time (they are calculated via RRFE matrix-reduction on a per-equation basis) they have to be initialized from a f32

Well, this is the tricky part. Fraction is a lossless type. f32 is a lossy type. We cannot represent 1/3 by a float (although we can by a fraction). Thus, when you convert a f32 value into a Fraction value, you end up not with 1/3, but with 0.33333334/100000000, which is optimized via LCD and becomes 16666667/50000000. The fraction library cannot guess about the lost data and convert 0.3 or 0.33 to 1/3, because for the fraction those look like 3/10 and 33/100.

I thought that might be the case. I might have to rethink the way I do this.

Thanks for the help and responses!

dnsl48 commented 2 years ago

Sure, good luck with your project! I'll close this ticket for now, but please feel free to reopen it or create a new one if you'd like to talk :)