all_equal() is broken

[ceball]

Unless I'm missing something?

>>> from param.parameterized import all_equal
>>> all_equal([1],[])
True
>>> all_equal([1],[0])
False
>>> all_equal([1,2],[1])
True
>>> all_equal([1],[1,2])
True

def all_equal(arg1,arg2):
    """
    Return a single boolean for arg1==arg2, even for numpy arrays
    using element-wise comparison.
    Uses all(arg1==arg2) for sequences, and arg1==arg2 otherwise.
    If both objects have an '_infinitely_iterable' attribute, they are
    not be zipped together and are compared directly instead.
    """

Why return all(a1 == a2 for a1, a2 in zip(arg1, arg2))?

commit d767150659da4e526d53b10c011deb0a15ed3f97
Author: Dobromir Stefanov <[email protected]>
Date:   Tue Mar 27 23:07:54 2012 +0000

    API cleanup for param and topo.base

-        return all(arg1==arg2)
+        return all(a1 == a2 for a1, a2 in zip(arg1, arg2))

https://github.com/ioam/svn-history/commit/d767150659da4e526d53b10c011deb0a15ed3f97

[jbednar]

Is your objection that it should be verifying that the lengths are the same, because zip does not do that?

[ceball]

Yes, mismatched lengths is one objection. Is there some reason it was intended to only check elements up to the shortest one? I'd probably expect the shapes to be the same.

Another (from #180) is that it doesn't work for >1d arrays:

>>> a = np.array([[1,2],[3,4]])
>>> all_equal(a,a)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/cball/code/ioam/param2/param/parameterized.py", line 144, in all_equal
    return all(a1 == a2 for a1, a2 in zip(arg1, arg2))
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

[jbednar]

I'm sure that would have been an oversight, not a design feature.

[ceball]

One feature provided by this all_equal function is to suppress Parameterized names almost all the time when doing script_repr.

>>> param.parameterized.all_equal('Parameterized00001','Parameterized')
True

means

>>> param.parameterized.all_equal(param.Parameterized().name,param.Parameterized.name)
True 

means

>>> param.Parameterized().get_param_values(onlychanged=True)
[]

means

>>> param.Parameterized().script_repr()
'param.parameterized.Parameterized()'

And if you specify a name for the Parameterized, it will then appear - as expected:

>>> param.Parameterized(name='para').get_param_values(onlychanged=True)
[('name', 'para')]
>>> param.Parameterized(name='para').script_repr()
"param.parameterized.Parameterized(name='para')"

Great :)

Unless you pick a name that's some fraction of the class name :(

>>> param.Parameterized(name='Para').get_param_values(onlychanged=True)
[]
>>> param.Parameterized(name='Para').script_repr()
'param.parameterized.Parameterized()'

[jbednar]

Good points. Seems like we shouldn't be relying on a bogus all_equal to be able to achieve that behavior.

[analog-cbarber]

So to be clear get_param_values(onlychanged=True) is also broken.

Ugh. This just cost me an hour.

[jbednar]

I think we should (a) rename the current all_equal to something that conveys what it does, e.g. prefix_matches, (b) write a new all_equal that verifies that array shapes are the same and only then checks if the values are the same, and (c) check out all cases where we currently use all_equal and replace some of them with prefix_matches if they are relying on the prefix-matching behavior in an appropriate way, and otherwise using the better behavior.

@analog-cbarber , it sounds like you are reporting a specific issue, which is that it's not detecting a change in some cases, such as adding an additional item to a list? If so I think steps a,b,c would deal with that problem, but someone would need to do them!

[analog-cbarber]

Yes, that is right. I was trying to grab the non-default params to save persistent state and the name parameter was a prefix of the default name based on the class.

I just wrote my own version of get_param_values as a workaround.

[jbednar]

And in this case you did want to preserve the name parameter, even though it was just the default, auto-generated name? Or do you mean that you had explicitly set the name, but it happens to be the same as the class name, which caused it not to be saved?

[analog-cbarber]

Actually in my case the name is always being set and sometimes was set to a prefix of the class name (e.g. 'Radar' and RadarViewer), but I think this would happen with any string parameter where either the instance value or class default is a prefix of the other.

[tonyfast]

Perhaps a solution is to apply zip_longest as an iterator for comparison.

[jbednar]

Closing this 6 years after it was opened, to within a few days! We are persistent, if not timely.