dsharlet / LiveSPICE

Real time SPICE simulation for audio signals
MIT License
419 stars 61 forks source link

Target net6.0-windows everywhere #176

Closed dsharlet closed 1 year ago

dsharlet commented 1 year ago

Currently, we have a mess of target frameworks, some projects target multiple frameworks. I am getting some warnings about some of them being deprecated. Before doing a new release (#172) I thought I'd try to clean this up.

I'm currently trying to test using only net6.0-windows everywhere, to match the VST plugin. It seems to mostly be working, though it did uncover a bug that will take some effort to fix. Something we try to sort in computer algebra has a bad comparer implementation... this might explain some of the struggles @Federerer has mentioned in the system of equations solver.

@mikeoliphant do you know of any reason not to switch to net6.0-windows everywhere?

Federerer commented 1 year ago

@mikeoliphant do you know of any reason not to switch to net6.0-windows everywhere?

Maybe I can answer this, as I did the switch to net6.0 some time ago and didn't noticed any issues, so I think it should be safe

it did uncover a bug that will take some effort to fix. Something we try to sort in computer algebra has a bad comparer implementation...

I thought that I've reported it long time ago, but I can't find it now 🤔 To workaround this exception I switched to different sorting method, but it only hides the underlying problem, so it's not something I would like to call a fix 😉 this is what I did in Multiply and Add classes:

public static new Expression New(IEnumerable<Expression> Terms)
{
    Debug.Assert(!Terms.Contains(null));

    // Canonicalize the terms.
    var terms = FlattenTerms(Terms).ToArray();

    switch (terms.Length)
    {
        case 0: return 0;
        case 1: return terms.First();
        default:
            Array.Sort(terms);
            return new Add(terms);
    }
}

BTW, this might be the reason why in some cases Factor() ends up in an infinite loop, switching back and forth between two versions of the expression.

mikeoliphant commented 1 year ago

I think having everything target .NET 6.0 makes sense.

dsharlet commented 1 year ago

Sorry, you may have reported it and I missed it. Our CompareTo implementation really isn't even close to meeting the requirements of the net6.0-windows OrderBy implementation. This fails:

            Expression a = "x^2";
            Expression b = "-x";
            int ab = a.CompareTo(b);
            int ba = b.CompareTo(a);
            Debug.Assert(ab == -ba);

Ordering expressions really is a very hard problem, and I'm not sure it's possible to implement a CompareTo that satisfies the new OrderBy. Your workaround might be the best way to go.

OrderBy is supposed to be a stable sort, while Array.Sort is an unstable sort. I think that's not good either. That actually seems like it could also be the source of infinite loops in Factor (and probably other things too).

I'm going to try to figure out a way to make a conservative but safe CompareTo for Expression so we can use the (stable sorting) OrderBy.

dsharlet commented 1 year ago

I've realized that CompareTo was simply being overly clever. By making it less clever, I think this problem is fixed, and we can update to .net6.0-windows. I'm going to push a branch shortly to do this.