Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ComponentTensor needs at least an index to have an extent #3825

Open
ReubenHill opened this issue Jul 14, 2021 · 10 comments
Open

ComponentTensor needs at least an index to have an extent #3825

ReubenHill opened this issue Jul 14, 2021 · 10 comments
Labels

Comments

@ReubenHill
Copy link
Contributor

ComponentTensor makes the below assertion:

class ComponentTensor(Node):
    __slots__ = ('children', 'multiindex', 'shape')
    __back__ = ('multiindex',)

    def __new__(cls, expression, multiindex):
        assert not expression.shape

        # Empty multiindex
        if not multiindex:
            return expression

        # Collect shape
        shape = tuple(index.extent for index in multiindex)
        assert all(s >= 0 for s in shape) ## <- HERE

which cannot be done if none of the indices have an extext.

This typically comes up when creating indices with gem.indices which do not set the extent property of the indices.

@wence-
Copy link
Contributor

wence- commented Jul 14, 2021

ComponentTensor turns free indices into shape. To do this, the indices must have an extent (otherwise what is the resulting shape?)

@ReubenHill
Copy link
Contributor Author

Perhaps the issue is upside down then and what one really wants is gem.indices to be able to set extent

@wence-
Copy link
Contributor

wence- commented Jul 14, 2021

So ComponentTensor requires that multiindex <= expression.free_indices, so if you do:

i, j = indices()
...
ct = ComponentTensor(expr, (i, j))

What is the situation under which expr has i and j as free indices, but i and j have not had their extents set? I think it is only if you did something like:

expr = Delta(i, j)...

Is that what you're doing?

@ReubenHill
Copy link
Contributor Author

Ah I see - if I use Indices to index expressions their extent is modified:

indices = gem.indices(len(T.shape))
assert indices[0].extent is None # true
T[indices]
assert indices[0].extent is None # false! Causes assertion error.

I wasn't expecting this.

What is the situation under which expr has i and j as free indices, but i and j have not had their extents set? I think it is only if you did something like:

expr = Delta(i, j)...

Is that what you're doing?

Yes it is.

@wence-
Copy link
Contributor

wence- commented Jul 14, 2021

Ah I see - if I use Indices to index expressions their extent is modified:

indices = gem.indices(len(T.shape))
assert indices[0].extent is None # true
T[indices]
assert indices[0].extent is None # false! Causes assertion error.

I wasn't expecting this.

Ah, OK, Indexed does:

        for index, extent in zip(multiindex, aggregate.shape):
            assert isinstance(index, IndexBase)
            if isinstance(index, Index):
                index.set_extent(extent)

And Index.set_extent says:

    def set_extent(self, value):
        # Set extent, check for consistency
        if self.extent is None:
            self.extent = value
        elif self.extent != value:
            raise ValueError("Inconsistent index extents!")

Is that what you're doing?

Yes it is.

In this case I would do:

i, j, k = (Index(extent=n) for n in some_shape)

Since I assume you have the shape lying around somewhere.

@wence-
Copy link
Contributor

wence- commented Jul 21, 2021

Is this OK to close?

@ReubenHill
Copy link
Contributor Author

Well Indexed silently changing index.extent feels rather naughty to me. Though I note that the Indexed class and the ComponentTensor class have docstrings. I suppose this is really a sub-issue of #3826 .

@wence-
Copy link
Contributor

wence- commented Jul 21, 2021

It's a user-interface "nicety". Indices need to carry their extent, but sometimes it's easier to just make them before you know the extent they'll need and then have it set on use (note that the extent is only ever changed from None to n and subsequently if an index is used we check that the extent and the use matches).

@ReubenHill
Copy link
Contributor Author

Well Indexed silently changing index.extent feels rather naughty to me. Though I note that the Indexed class and the ComponentTensor class have docstrings. I suppose this is really a sub-issue of #3826 .

Apologies this was meant to say that they don't have docstrings

@ReubenHill
Copy link
Contributor Author

The answer seems to me to be that if you let indices have no extent then it should be very clearly documented somewhere that this will be set upon use

@connorjward connorjward transferred this issue from firedrakeproject/tsfc Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants