Check for `is_normalized` for `NumericalTarget` is too strict

[AVHopp]

Currently, the check for verifying whether or not a target is normalised is too strict as it checks for Interval(0,1) exactly. This causes issues when using Transformations that do not exactly calculate Interval(0,1) but something close to it like Interval(0,0.999999999) like in the following example:

from baybe.targets import NumericalTarget

t_clamp_bug = NumericalTarget.match_bell(name = "Bug", match_value=5, sigma=0.1).clamp(0, 0.606531).normalize()
print(t_clamp_bug.get_image((-100, 100)))

> Interval(lower=0.0, upper=0.9999999999999999)

To fix this, we need to use a tolerance or a fuzzy match when checking whether or not a target is normalised.

[Scienfitz]

likely not really a problem of NumericalTarget but of Interval

Interval.lower and Interval.upper need a custom eq overwrite like

from attrs import cmp_using

class Inteval(...):
    lower: float = field(..., eq=cmp_using(eq=np.isclose))
    upper: float = field(..., eq=cmp_using(eq=np.isclose))

np.isclose is the method of choice and is already used in other parts of the code

[AdrianSosic]

likely not really a problem of NumericalTarget but of Interval

I'm currently torn between association the issue with Interval vs with NumericalTarget. The fix that @Scienfitz suggested certainly works an is perhaps the right way to go. But I could also argue that approximations/rounding/discretization should only ever applied at the very end, in the place where the rounded value is needed. In this spirit, the consequence would be to leave the general interval comparison with whatever it currently spits out and apply the isclose version only to the specific comparison taking place in is_normalized, since the latter is the Boolean quantity that needs to produce a discretized variable.

Thoughts?

[Scienfitz]

So the topic here is basically floating point representation in the computer. we cannot change that paths that analytically would yield the exact same number do not lead to the same number in the computer (example is the original issue here)

The only reasonable consequence of that for me is, that floating point comparison with == actually does not make sense, it should simply always be isclose (perhaps everywhere in all our classes), because the problem described above can happen any and everywhere without us controlling it.

This argument is in line with my suggested fix and completely free of thinking about how higher level functions compare floats or objects that contain floats. Are there any arguments against it?

Now whats not so clear to me is whether we can find generally reasonable atol and rtol values.

[AdrianSosic]

Well, one could argue that if isclose is always the "right" thing to do, then why

>>> (0.1+0.2) == 0.3
False

I think using isclose as a default is too invasive, and also causes many other problems (i.e. how to set atol/rtol depending on used machine precision etc)? It would also imply that for consistency we'd have to patch all our attributes in classes that have a regular float type --> seems very wrong to me.

For the interval, I think it's OK to make an exception if you insist, but I would feel better with trying my suggested less invasive approach of patching is_normalized first and only go the extra mile of touching the Interval logic itself if we encounter more problems in the future.

[AVHopp]

First of all, can we agree that this PR should only fix the currently existing issue and that we do NOT start implementing whatever solution we come up with here already in this PR? Otherwise, this will blow up and take for ages again, and I personally would prefer smaller PRs, each fixing a single problematic comparison (potentially onto a feature branch).

And to be honest: Given that we check membership in an interval simply by using isclose, we need to use this for consistency reasons, as currently, we have the following logic already in place:

>>> Interval(0,0.999999).contains(1)
True

And to me, this implies that Interval(0,0.9999999) has to be equal to Interval(0,1) according to our own logic. So I'd say using isclose here is the way to go, and that the topic might be revisited in more generality later.

[Scienfitz]

@AdrianSosic not sure what point you were trying to make with that example but you made exactly my point: The == operator is flawed (not in itself but because of floating point prepresentaiton), because we know for a fact 0.1+0.2==0.3 is true analytically. isclose would have returned the correct result, while == doesnt. Thus if we dont use isclose for all float comparisons we accept a certain amount of cases where the result is simply wrong thanks to incorrect float comparisons (the original issue here is an example).

Given the point above, why would isclose be too invasive? It would actually not be that hard to overwrite the eq for all float attributes with a utility that consists of an appropriate pre-assembled cmp_using etc

[Scienfitz]

ok so what to do about it? if we want to skip the general float problem then is_normalized should just be fixed like

np.isclose(interv.lower,0) and np.isclose(interv.upper,1)

[AVHopp]

I'd still say that it would be weird for the code base to say that Interval(0.0.99999999) contains 1 while having Interval(0,0.999999) != Interval(0,1), so I would still argue that this should be fixed as it currently is done in the PR. If both of you disagree we can also just fix is_normalizedthough.

[Scienfitz]

@AVHopp but if just is_normalized is changed and everything else remains untouched we will have:

• 1 will not be in Interval(0,0.999999999999)
• Interval(0,0.999999) != Interval(0,1) will be True

What is inconsistent about that?

[AVHopp]

No, according to our code logic that already exists, 1 is contained in Interval(0,0.99999999), this is our current Interval.contains function which already uses np.isclose:

def contains(self, number: float) -> bool:
        """Check whether the interval contains a given number.

        Args:
            number: The number that should be checked.

        Returns:
            Whether the interval contains the number.
        """
        return (
            bool(np.isclose(number, self.lower))
            or bool(np.isclose(number, self.upper))
            or (self.lower < number < self.upper)
        )