max_cc_value > 1.0 on some machines

[wjlei1990]

Hi,

I recently discover pyflex is generating max_cc_value > 1.0 on one window.

One one machine:

win1 = Window(left=1783, right=2032, center=1907, channel_id=IU.KBL..BHZ, max_cc_value=1.00000011921, cc_shift=0, dlnA=0.0)

It is just slightly larger than 1.0. I discovered it because I am using pytest to do some tests and I am comparing the results generating from another machine.

On another one(the benchmark result):

win2 = Window(left=1783, right=2032, center=1907, channel_id=IU.KBL..BHZ, max_cc_value=1.0, cc_shift=0, dlnA=0.0)

It is a bit strange since theoretically, max_cc_value should be =< 1.0. Not sure it is a machine precision issue, an numpy issue or algorithm issue.

[krischer]

Hmm..I'd argue its due to numpy. This is the code responsible and looks quite right to me:

https://github.com/krischer/pyflex/blob/b3168622b064f96f25902cd4d575335da9f09e15/src/pyflex/window.py#L224-L229

We could just clip the value, e.g. cc = np.clip(cc, -1.0, 1.0) to work around that issue. Also the difference is small enough that it likely does not matter.

[wjlei1990]

Yes, should not be a big issue. Though brutally apply np.clip() makes me a little uncomfortable :) I think it is fine.

In you pyflex.Window, you have "eq" method, which strictly compares everything.

    def __eq__(self, other):
        return self.__dict__ == other.__dict__

I am using assert window1 == window2, so it compares every number very strictly. Do you think it is a good comparison?

[krischer]

Yes, should not be a big issue. Though brutally apply np.clip() makes me a little uncomfortable :) I think it is fine.

Yea me too...but in this case I'm quite confident the logic is correct.

Do you think it is a good comparison?

No that's not a good one at all especially for floating point numbers so feel free to change it to something a bit better! np.testing has utilities to test floating points values.

[krischer]

Just change the __eq__() method.

[wjlei1990]

Got it.

[wjlei1990]

Sort of dirty fix?

    def __eq__(self, other):

        def _compare_value(value1, value2):
            if isinstance(value1, float) or isinstance(value1, np.float64):
                np.testing.assert_allclose(value1, value2)
            elif isinstance(value1, np.float32):
                np.testing.assert_allclose(value1, value2, rtol=1e-06)
            else:
                if value1 != value2:
                    raise AssertionError

        dict1 = self.__dict__
        dict2 = other.__dict__
        try:
            for key in dict1.keys():
                if key == "phase_arrivals":
                    phase1 = dict1[key]
                    phase2 = dict1[key]
                    if len(phase1) != len(phase2):
                        raise AssertionError
                    for _p1, _p2 in zip(phase1, phase2):
                        for _key_phase in _p1.keys():
                            _compare_value(_p1[_key_phase], _p2[_key_phase])
                else:
                    _compare_value(dict1[key], dict2[key])
        except Exception as err:
            return False
        return True

I have done it in my devel branch on my repo.