Fixing bug which causes multiplication by Probability to return a Probability

[jmbarr]

In general when you multiply a value by a probability you shouldn't get a probability. This is what the overwritten __mult__() method is doing. Hence if a = Probability(0.3)*5.0, a = Probability(1.5) which doesn't make sense. This fixes that to return a float unless the input is Probability in which case return Probability.

There may also be a discussion to be had about preserving the class of the value by which the probability is multiplied, using value.__class__(prob*value) or something, but that runs a little against allowing Python to do its thing. And it creates problems with integers. But it might fix a problem by which StateVectors of size (1,1) are reduced to floats.

[sdhiscocks]

What are your thoughts on having different behaviour for __mul__ and __rmul__?

[jmbarr]

What are your thoughts on having different behaviour for __mul__ and __rmul__?

Hadn't considered it (because I didn't have any concept of __rmul__) but yes, $p \times a$ should be the same as $a \times p$ -- both should return type(a), or at least a float. So need to adapt __rmul__ too.

[jmbarr]

What are your thoughts on having different behaviour for __mul__ and __rmul__?

Hadn't considered it (because I didn't have any concept of __rmul__) but yes, p×a should be the same as a×p -- both should return type(a), or at least a float. So need to adapt __rmul__ too.

Which is how it works now as, it seems ,__rmul__ just invokes __mul__. As multiplying by probabilities will always commute, no further changes necessary.

[sdhiscocks]

What are your thoughts on having different behaviour for __mul__ and __rmul__?

Hadn't considered it (because I didn't have any concept of __rmul__) but yes, p×a should be the same as a×p -- both should return type(a), or at least a float. So need to adapt __rmul__ too.

Which is how it works now as, it seems ,__rmul__ just invokes __mul__. As multiplying by probabilities will always commute, no further changes necessary.

I wonder if it would make sense to have __mul__ maintain the Probability type, as it's leading, and then have __rmul__ maintain the other (or float) as it's leading?

This would then mean Probability(0.1) * 0.2 would yield Probability still which I can see use case for, but 0.2 * Probability(0.1) would return float.

[jmbarr]

What are your thoughts on having different behaviour for __mul__ and __rmul__?

Hadn't considered it (because I didn't have any concept of __rmul__) but yes, p×a should be the same as a×p -- both should return type(a), or at least a float. So need to adapt __rmul__ too.

Which is how it works now as, it seems ,__rmul__ just invokes __mul__. As multiplying by probabilities will always commute, no further changes necessary.

I wonder if it would make sense to have __mul__ maintain the Probability type, as it's leading, and then have __rmul__ maintain the other (or float) as it's leading?

This would then mean Probability(0.1) * 0.2 would yield Probability still which I can see use case for, but 0.2 * Probability(0.1) would return float.

But multiplying by a probability should be commutative -- how would someone know to expect this (non-standard I would argue) behaviour?

And it would still be wrong in the other way... e.g. Probability(0.1) * 20 would yield Probability(2) which doesn't make sense.

(What's the use case you're thinking of?)

[sdhiscocks]

But multiplying by a probability should be commutative -- how would someone know to expect this (non-standard I would argue) behaviour?

Uh, sort of... the type of left takes priority using __mul__, unless it doesn't implement it the type on the right __rmul__ is called instead. You also have things like this: If the right operand’s type is a subclass of the left operand’s type and that subclass provides a different implementation of the reflected method for the operation, this method will be called before the left operand’s non-reflected method. This behavior allows subclasses to override their ancestors’ operations.

(What's the use case you're thinking of?)

I was thinking were you provide probabilities as floats in some options (as must easier to write and don't care about floating point issues with large values). There's one example here, where pdf is Probability type (L153), but self.prob_detect(L164) might not be: https://github.com/dstl/Stone-Soup/blob/89b8782120ac4784eb44c16387ef3792180b4c29/stonesoup/hypothesiser/probability.py#L153-L164

I wonder if better solution would be your changes here, plus implement __imul__ to maintain type? The above would be fixed with probability = pdf; pdf *= self.prob_detect (in fact, just use one variable, pdf or probability as don't need both). Plus you would expect type to be maintained with __imul__ where possible for sure.

[jmbarr]

I wonder if better solution would be your changes here, plus implement __imul__ to maintain type? The above would be fixed with probability = pdf; pdf *= self.prob_detect (in fact, just use one variable, pdf or probability as don't need both). Plus you would expect type to be maintained with __imul__ where possible for sure.

Not sure I follow all of this and I think it's getting a bit complicated. (If I understand the example correctly, the product is two Probabilitys (despite one actually being a pdf) so the result would be Probability also.)

But fundamentally, the order of the products shouldn't matter, and if we make it matter then we'll have to explain it to users who probably won't be looking out for it.

As I said previously, choosing to preserve type may be too problematic (e.g. multiplying a Probability by an integer would yield an integer), so a float, or array of floats likely makes the most sense..

[sdhiscocks]

I wonder if better solution would be your changes here, plus implement __imul__ to maintain type? The above would be fixed with probability = pdf; pdf *= self.prob_detect (in fact, just use one variable, pdf or probability as don't need both). Plus you would expect type to be maintained with __imul__ where possible for sure.

Not sure I follow all of this and I think it's getting a bit complicated. (If I understand the example correctly, the product is two Probabilitys (despite one actually being a pdf) so the result would be Probability also.)

pdf isn't the right variable name in this case, which may be causing confusion?

But fundamentally, the order of the products shouldn't matter, and if we make it matter then we'll have to explain it to users who probably won't be looking out for it.

The value should be the same, but the type could vary depending as mentioned. This is already expected behaviour of the language. And by default the type shouldn't change, so by making it a float by default is something that users probably won't be looking out for it.

As I said previously, choosing to preserve type may be too problematic (e.g. multiplying a Probability by an integer would yield an integer), so a float, or array of floats likely makes the most sense..

I'm not suggesting preserving the type, other than Probability or float, and there is appropriate use cases for both.

[jmbarr]

The value should be the same, but the type could vary depending as mentioned. This is already expected behaviour of the language. And by default the type shouldn't change, so by making it a float by default is something that users probably won't be looking out for it.

I'm not sure as a casual Python user I'd expect Probability($x$) $\times$ $y =$ Probability($xy$).

I'm not suggesting preserving the type, other than Probability or float, and there is appropriate use cases for both.

Agree with that but the only case I can think of where the output type is Probability is where both inputs are Probability.

More broadly, given that the intent of this type (correct my if I'm wrong) is just to allow calculation over a greater dynamic range, rather than strict conformance to the axioms of probability, perhaps calling it "Probability" was inadvisable.

[sdhiscocks]

The value should be the same, but the type could vary depending as mentioned. This is already expected behaviour of the language. And by default the type shouldn't change, so by making it a float by default is something that users probably won't be looking out for it.

I'm not sure as a casual Python user I'd expect Probability(x) × y= Probability(xy).

I'm not suggesting preserving the type, other than Probability or float, and there is appropriate use cases for both.

Agree with that but the only case I can think of where the output type is Probability is where both inputs are Probability.

More broadly, given that the intent of this type (correct my if I'm wrong) is just to allow calculation over a greater dynamic range, rather than strict conformance to the axioms of probability, perhaps calling it "Probability" was inadvisable.

Yes. But certainly has it's own issues when greater dynamic range isn't important.

I think __imul__ (returning a new instance) is going to be required to get around some unexpected behaviour, but otherwise happy with the change.