SetExpr (from mrocklin) with multipledispatch

[Upabjojr]

This PR merges two open PRs:

• Interval arithmetic operations #11615
• Set Expressions #2721

At the same time, a copy of multipledispatch is added as a SymPy submodule. The logic of applying operations on sets is then implemented through multiple dispatching on the types of sets (and expressions).

It also closes #14062

The original PR #2721 contained a system to distinguish among identical set expressions and equivalent set expressions. For example, if a and b are both Interval(1, 2), then

1. a-a == FiniteSet(0)
2. a-b == Interval(-1, 1)

The logic being that in case 1. the element is drawn from the same set. I have omitted to implement this logic, as I think it might be confusing.

[Upabjojr]

@mrocklin

[mrocklin]

I see that sympy still isn't accepting external dependencies these days?

[Upabjojr]

I see that sympy still isn't accepting external dependencies these days?

I would support external dependencies, but there are complications for packaging.

[mrocklin]

That was certainly the stance a long time ago when I was active with SymPy. I had hoped that the project would have changed its policy. Oh well.

…

On Wed, Feb 7, 2018 at 12:40 PM, Francesco Bonazzi ***@***.*** > wrote: I see that sympy still isn't accepting external dependencies these days? I would support external dependencies, but there are complications for packaging. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14106 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASszLV6BxG8kwGuvUVUqKdGiucLbqHmks5tSd_4gaJpZM4R9Eec> .

[Upabjojr]

mpmath is now an external dependency.

[Upabjojr]

@hargup @gxyd

[skirpichev]

I would support external dependencies, but there are complications for packaging.

setuptools handle external deps well.

I had hoped that the project would have changed its policy. Oh
well.

It make sense sometimes. Unfortunately, not all your project are supported. But I don't think multipledispatch is the case.

@Upabjojr, I see you workaround problem with distributivity (e.g. x + x = 2*x in Add.flatten) by wiping out using Mul/Add? There are MatrixExpr subclasses (MatAdd and so on), which also reimplement such stuff from the core. Maybe more (tensors module?). You did it second (or third) time)

[Upabjojr]

I see you workaround problem with distributivity (e.g. x + x = 2*x in Add.flatten) by wiping out using Mul/Add?

Well, it was not my intention to address the distributivity issue in this PR. The add_sets dispatcher should not be interpreted as a constructor for Add. It simply creates the set that would result by adding all pair-combinations from the two sets (being added).

Maybe more (tensors module?)

Not yet implemented there.

The distributivity is an interesting issue, but not for this PR.

[asmeurer]

The logic being that in case 1. the element is drawn from the same set. I have omitted to implement this logic, as I think it might be confusing.

So which of the two does it do? The second one is the most correct.

[asmeurer]

I think we should find some better way to pretty print set expressions. Right now they print just like their underlying sets, and they even combine greedily with other sets (SetExpr + Set = SetExpr). This is very confusing since the arithmetical operations on sets are very different from the arithmetical operations on set expressions. If we can't come up with anything clever we should just pretty print SetExpr([1, 2]).

We also should add something like as_expr to Set, or at the very least add SetExpr to the global namespace.

[Upabjojr]

So which of the two does it do? The second one is the most correct.

It's not implemented like that. This expression is commented out. The correctness is discutibile, one may reason for both ways.

[asmeurer]

In order to have a notion of sets being the "same" or not, you have to have to include something else alongside the set. Equal sets are equal. That's both mathematically true and how SymPy works.

The difference here is between {x - y | x in a, y in a} and the diagonal {x - x | x in a, x in a}. The diagonal is obviously contained in the general case (subset). For full generality, the set expression construction should use a different dummy variable for each operand.

It won't be possible to get a wrong result from doing it this way, although you may get something that is larger than you wanted. The original motivation here is to do interval arithmetic. If f(x) = sin(x) and g(x) = cos(x) both are contained in the interval [-1, 1], then it is not wrong that f - g and f - f are contained in [-2, 2] (actually neither case is tight). However, it is wrong if we give {0} in the f - g case. The issue of course is that if we let a = [-1, 1], there is nothing tying a to f or to g, so without that information we have to assume that each instance of a came from a different function.

The primary issue here is that SymPy doesn't have a way to distinguish two identical sets. Perhaps a clearer solution would be to add a name to a set expression. This would make it similar to the random variable case, where X - X is 0 but X - Y is not, even if X and Y are iid. Perhaps this would clear up the printing issue I mentioned as well.

We could also go at the interval arithmetic directly and associate an arbitrary expression with a SetExpr, with SetExpr(expr, set) meaning that expr is bounded by set. Two SetExprs are not equal unless they have the same set and expr. This is just an idea (not sure yet if it's a good one).

I'm really curious how the original code attempted to fix this issue.

[Upabjojr]

I'm really curious how the original code attempted to fix this issue.

@mrocklin had a counter variable registered in the args of SetExpr. At this point, with the same set and the same counter, a - a == FiniteSet(0) for any a = SetExpr(...). That is, if both set and counter match, build {x - x | x in a}, otherwise build {x - y | x in a, y in a}.

The alternative of using a name for SetExpr as it is done in RandomSymbol could also work, but maybe it's too cumbersome.

Otherwise we could ignore the diagonal set construction, as always assume that the elements are drawn from two sets (which is the case in this PR).

[Upabjojr]

Any objections to merging this?

[moorepants]

Just wanted to note that I think we shouldn't bundle multipledispatch. We should treat it as a dependency like mpmath. Seems like we are going backwards. We decided to unbundle mpmath and opened the doors having required dependencies and now are not using that freedom.

[Upabjojr]

Well, including it as a subfolder was the fastest thing to do. We can unbundle it as soon as someone feels like opening a PR.

[mrocklin]

I'm surprised that vendoring a library was faster than importing it and adding it to the requirements. Is there a plan to merge in updates as they happen upstream? Is there a plan to contribute patches in sympy to the upstream version?

[asmeurer]

@mrocklin is multipledispatch still actively developed? It didn't look like there had been much activity in a while when I looked at the GitHub.

[mrocklin]

@llllllllll and @mariusvniekerk do some work on it from time to time.

But even if the answer was "no", wouldn't it be better for sympy developers to maintain it out in the open rather than inside the sympy codebase?

[gxyd]

I think we should also need to have an entry for this in release notes https://github.com/sympy/sympy/wiki/Release-Notes-for-1.2#new-features . Also now that we've multipledispatch as well, perhaps that should go in the major changes?

[Upabjojr]

Also now that we've multipledispatch as well, perhaps that should go in the major changes?

Agreed, but I would recommend to stress the fact that this feature is quite new and probably contains some number of bugs. Hopefully they will get corrected over time.

[moorepants]

Well, including it as a subfolder was the fastest thing to do.

If we keep this, shouldn't the entire package and its functions be preprended with an underscore so that it is essentially private and we don't have people relying on the API externally?

[Upabjojr]

It is currently hidden, i.e. it does not show up in the documentation. I would suggest to keep it until Python 2.7 support is dropped in both SymPy and multipledispatch, which is a possible issue that may arise from external dependencies in the next few years.