rust-or / rust-lp-modeler

Lp modeler written in Rust
MIT License
96 stars 29 forks source link

fix stack overflow error and reduce public API (rebased #61) #68

Closed lovasoa closed 3 years ago

lovasoa commented 3 years ago

I merged master into #61 and fixed the conflicts

lovasoa commented 3 years ago

I tried to add a test to see if it fixes #37, but it looks like it doesn't :

    #[test]
    fn simplify_deep_sum() {
        let count = 1000;
        let vars: Vec<LpExpression> = (0..count)
            .map(|i|
                &LpInteger::new(&format!("v{}", i)) * 2 + 1
            )
            .collect();

        let mut sum = lp_sum(&vars);
        let c = sum.simplify().split_off_constant();
        assert_eq!(c, count as f32);
    }

So it looks like it signicantly increases complexity, without fixing the initial issue :/

lovasoa commented 3 years ago

It looks like simplify repetitively calls show, which is a recursive function that allocates a large string !

lovasoa commented 3 years ago

@dlaehnemann @jcavat : I removed the recursivity in Show. It is still super slow (allocating a ton of strings during simplify is not a good idea), but at least it works now, it does not throw a stack error.

jcavat commented 3 years ago

Great job guys! I merge and wait few days before upgrading to version 0.4.4 (or 0.5.0). (I will be off next week)