fuzzylite / pyfuzzylite

pyfuzzylite: a fuzzy logic control library in Python
https://fuzzylite.com/
Other
65 stars 14 forks source link

Bug in SmallestOfMaximum? #9

Closed CJxD closed 2 years ago

CJxD commented 2 years ago

I was just reading through the defuzzifiers, when I noticed that the code for SmallestOfMaximum is the same as LargestOfMaximum. The only difference is the initial value of x_smallest. I think the for-loop is supposed to go from resolution down to 0 rather than 0 to resolution.

Am I missing something?

    def defuzzify(self, term: Term, minimum: float, maximum: float) -> float:
        if not math.isfinite(minimum + maximum):
            return nan
        resolution = self.resolution
        dx = (maximum - minimum) / resolution
        y_max = -math.inf
        x_smallest = minimum
        for i in range(0, resolution):
            x = minimum + (i + 0.5) * dx
            y = term.membership(x)
            if Op.gt(y, y_max):
                y_max = y
                x_smallest = x
        return x_smallest

https://github.com/fuzzylite/pyfuzzylite/blob/28c75b32e0f1b69afe0fb7a4b70f61952929f260/fuzzylite/defuzzifier.py#L186

jcrada commented 2 years ago

Hi Chris,

Thanks for your report.

The code is actually fine. The difference is in the comparison if Op.gt(y, y_max): for smaller of maximum, and if Op.ge(y, y_max): for largest of maximum, that is, Op.gt: greater than and Op.ge: greater than or equal to.

Say you have this:

      ______________
_____/              \_____

The smaller of maximum should get the first (left to right) x coordinate where y is maximum, so you iterate and change only when you find a greater than value.

The largest of maximum should get the last (left to right) x coordinate where y is maximum, so you iterate and change when greater than or equals to.

This is maybe easier to see in QtFuzzyLite, just make sure to use Trapezoid terms instead of Triangle terms.

Does this answer your question?

Cheers.

jcrada commented 2 years ago

Oh, I see what you mean, Chris! Yes, it is actually a bit more efficient to do the same but in reverse 😅. The iteration would be instead:

            x = maximum - (i + 0.5) * dx
CJxD commented 2 years ago

Oh yes, that's a very subtle change to do Ge rather than Gt, didn't notice that! Makes sense.

I'm in the final stages of my "fuzzyverylite" extension after getting a bit more time to have another look. It uses the Python library to parse the FLL, and then spits out C++ code to perform the calculations. I've managed to replicate one of our models in only 23KB of binary so far. I'll send it your way when I'm done with it for academic interest!

Heads up, I noticed in my last push over Christmas that pyfuzzylite couldn't load one of our FLLs successfully. I'll make another bug report if the same still happens.