Confusion over type validation

[gmarkall]

There are things like this, for example in base.py:

 @validate_type(('maps', (Map, tuple), MapTypeError), \
                   ('dims', (int, tuple), TypeError))
    def __init__(self, maps, dims, name=None):

What is the type validation for maps supposed to mean? It looks as if the intention is to check that maps is a tuple of Map objects, but it is actually checking that maps is either a Map or a tuple of any type of objects. It is never OK to pass a single Map to a Sparsity, so I think the code is also just wrong.

Do we want to refine the type validation so that it can check for more complex specifications? e.g check that we're passing a list of lists of tuples of tuples of maps, which is what we'd need to check for the sparsity for a mixed space? Or, alternatively, do we want to just fix the sparsity type, validation, i.e.:

 @validate_type(('maps', (tuple), MapTypeError), \
                   ('dims', (int, tuple), TypeError))
    def __init__(self, maps, dims, name=None):

and leave it at that?

[wence-]

Later on in sparsity construction we do:

self._rmaps, self._cmaps = map (lambda x : as_tuple(x, Map), zip(*lmaps))

which will raise an error if we passed non-Map objects in as a tuple. So I think the latter is fine.

I note also in passing that a bunch of the Sparsity API tests pass, but are testing the wrong thing.

For example:

    def test_sparsity_illegal_rmap(self, backend, smap):
        "Sparsity rmap should be a Map"
        with pytest.raises(TypeError):
            op2.Sparsity('illegalrmap', smap, 1)

That's definitely not the correct call signature (with a bad Map argument) for Sparsity construction.

[kynan]

@gmarkall I think the intent of allowing to pass a single Map was that often it would be the case that both maps in the pair would be the same, so you could only pass one which would be used for both.

However this is currently broken because base.py:657 doesn't make any sense:

lmaps = (maps,) if isinstance(maps[0], Map) else maps

I think it wants to be:

lmaps = (maps,maps) if isinstance(maps, Map) else maps

Alternatively we could drop support for passing a single map by removing 657 and changing lmaps to maps in what is currently 658.

@wence- You're right, that's currently not correct call signature, since passing a single Map to the Sparsity constructor is broken. I think it used to work at some point.

Either way, we should decide what arguments we want to support, fix the Sparsity constructor and add a test to make sure we don't break it again.

[gmarkall]

@kynan I think line 657 is actually correct. The Sparsity constructor takes a tuple of 2-tuples of maps. However, in the case where there's a single pair of maps, there's a lot of brackets (Sparsity(((map, map)), ...)) so line 657 converts a pair of maps into a tuple containing a tuple with a pair of maps.

[kynan]

OK, that makes sense to me. So that means we don't want to add back support for passing in a single Map for convenience? In which case we should add a test to assert failure of passing a single Map to a Sparsity constructor. And the illegal syntax @wence- pointed out should be fixed. Will propose a patch.

[kynan]

Fixed in 4e12752.