change the default value of ignore_trivial param in stump

[NimaSarajpoor]

I was wondering if it would be reasonable to set ignore_trivial to default to None. Then, if user does not set it to True or False, we can set it automatically:

if ignore_trivial is None:
    if T_B is None:
          ignore_trivial = True
    else: 
          ignore_trivial = False

Because, I made a mistake once when I was trying to calculate AB-join. I then realized that I should have set ignore_trivial param to False.

[seanlaw]

It's probably fine. I'm just trying to think whether there are any situations where T_B is not None and ignore_trivial should be True?

[NimaSarajpoor]

@seanlaw
Apparently is checks for the id, and it was very interesting when I realized two False can have different ids !!!

Example 1

T_A = np.array([1, 2, 3, 0, 1, 2, 4, 0, 1, 2, 3.0, -4, -10, 1])
T_B = np.array([11, 12, 31, 10, 11, 12, 41, 10, 11.0, 21, 31.0, -41, -101, 11])
out = stumpy.core.are_arrays_equal(T_A, T_B)
>>> print(out) 
False

>>> id(out)
140733916434904

>>>id(False)
140734028742000

x = out is False
>>> x
False

Example 2

out = False
x = out is False
>>> x 
True

So, we just need to replace is with ==.

[seanlaw]

Ouch! I guess that check NEVER actually worked and it looks like we have this in multiple places when I check grep "is False" *py. 🤦‍♂️ Sorry about that!

As you've proposed, I think we should:

1. Replace is with ==
2. Refactor: Define a new function in core.py to check_ignore_trivial

Something like:

# core.py
def check_ignore_trivial(T_A, T_B, ignore_trivial):
    if ignore_trivial == False and core.are_arrays_equal(T_A, T_B):  # pragma: no cover
        logger.warning("Arrays T_A, T_B are equal, which implies a self-join.")
        logger.warning("Try setting `ignore_trivial = True`.")

    if ignore_trivial and core.are_arrays_equal(T_A, T_B) == False:  # pragma: no cover
        logger.warning("Arrays T_A, T_B are not equal, which implies an AB-join.")
        logger.warning("Try setting `ignore_trivial = False`.")

Or some reasonable combination of warnings that we described above. Then, finally, we add some unit tests to test this function to ensure that it does what we would expect.

What do you think? Personally, as annoying as it may be, I don't love ignore_trivial=None because we are now committing ourselves to inferring what the user wants. In most cases, we'll likely be okay but I am genuinely concerned about the rare edge cases where we would infer incorrectly. This is the whole "shooting yourself in the foot" that I am thinking of but maybe I am overthinking it. Again, I get annoyed by it too and, in the end, we should target the majority (obvious) use case so I am open to it.

[NimaSarajpoor]

Thanks for catching the other cases via grep. Yes... refactoring would be great!!!

Personally, as annoying as it may be, I don't love ignore_trivial=None because we are now committing ourselves to inferring what the user wants. In most cases, we'll likely be okay but I am genuinely concerned about the rare edge cases where we would infer incorrectly.

You are right... I have been thinking about it lately and I think the warning is better. Although this warning might annoy the person who want to intentionally set ignore_trivial=True in mp-join (because they are going to get warning again and again), I think it is safer and, as you once said, it somehow forces the user to THINK about what they are doing. Also, the case <ignore_trivial=True in mp-join> is rare and thus, that annoying thing is okay.

[seanlaw]

they are going to get warning again and again

Technically, they can turn that specific warning off if they don't care about it. There are straightforward ways to do this in Python

it somehow forces the user to THINK about what they are doing

Exactly! The warning is a gentle nudge to let the user know "hey, it looks like you are doing something that is atypical/rare so please take care but proceed carefully if you know what you are doing"

[NimaSarajpoor]

Technically, they can turn that specific warning off if they don't care about it. There are straightforward ways to do this in Python

My bad! I totally forgot about that!

---

I am going to open an issue shortly.

[seanlaw]

Tests should probably look something like:

def test_check_ignore_trivial():
    T_A = np.random.rand(10)
    T_B = np.random.rand(20)
    with warnings.catch_warnings(record=True) as w:
        ignore_trivial = False
        core.check_ignore_trivial(T_A, T_A, ignore_trivial)
        assert issubclass(w[-1].category.UserWarning)
        ignore_trivial = False
        core.check_ignore_trivial(T_A, T_B, ignore_trivial)
        assert issubclass(w[-1].category.UserWarning)

        assert "Arrays T_A and T_B" in str(w[-1].message)

https://stackoverflow.com/questions/3892218/how-to-test-with-pythons-unittest-that-a-warning-has-been-thrown

This will need to be added after the GPU stuff is cleared up