Logic around reset_sparsity parameter to assemble_system in python is flawed

[Martin Sandve Alnæs]

In dolfin/fem/assembling.py, the logic surrounding the reset_sparsity parameter is circular and inconsistent, and needs a review. I hit an exception (expected a tensor, when 'reset_sparsity' is 'False') but failed to reproduce it when extracting the code, so a rough analysis of the logic in the code is the best I can do with the time I have now. I think someone who knows this code needs to do a review and not just a single quick fix anyway, since it is not obvious in which part of the code to fix the bug.

Assume that the default value for reset_sparsity is used when calling assemble_system:

def assemble_system(A_form,
...
                    reset_sparsity=None,
...

It is first used in this line:
    (A_tensor, reset_sparsity) = _create_tensor(A_form, A_dolfin_form.rank(), backend, A_tensor, reset_sparsity)

where it will be replaced with False. Therefore, this call fails:
    (b_tensor, reset_sparsity) = _create_tensor(b_form, b_dolfin_form.rank(), backend, b_tensor, reset_sparsity)

because the logic that sees None and False as different values breaks down in _create_tensor.

Still in assemble_system, just below we find the lines:
    # Set default value for reset_sparsity if not specified
    if reset_sparsity is None:
        reset_sparsity = True
but this can probably never be true since reset_sparsity=None will be changed to False by the _create_tensor lines.

The logic in _create_tensor should probably differentiate between matrices and other tensors. I tried to do that, but just got other errors other places so I stopped digging.

Happy hunting from Italy!

[Anders Logg (logg)]

I have made an attempt to fix this, but please check that it makes sense. The logic is simplified now (reset_sparsity is just passed along to C++) as I don't see why it must be tied to the use of the cache. One should be able to use the cache and reset the sparsity pattern.

[Johan Hake (johan-hake)]

Why do we use a cache in the first place? Wouldn't it be more natural that a
user keep track of any already assembled tensors.

Also, my would a user want to use a cached Matrix _and_ reset the sparsity?

Johan

On Tuesday September 20 2011 07:17:05 Anders Logg wrote:
> I have made an attempt to fix this, but please check that it makes
> sense. The logic is simplified now (reset_sparsity is just passed along
> to C++) as I don't see why it must be tied to the use of the cache. One
> should be able to use the cache and reset the sparsity pattern.
>
>
> ** Changed in: dolfin
> Status: New => Fix Committed
>
> ** Changed in: dolfin
> Assignee: (unassigned) => Anders Logg (logg)

[Anders Logg (logg)]

On Tue, Sep 20, 2011 at 03:42:22PM -0000, Johan Hake wrote:
> Why do we use a cache in the first place? Wouldn't it be more natural that a
> user keep track of any already assembled tensors.

I don't know. I don't remember creating the cache. I thought you added
it?

> Also, my would a user want to use a cached Matrix _and_ reset the
> sparsity?

It may be stupid but it's up to the user. The old logic tried to
impose some restrictions like not resetting the tensor when it was
fetched from cache.

--
Anders

> Johan
>
> On Tuesday September 20 2011 07:17:05 Anders Logg wrote:
> > I have made an attempt to fix this, but please check that it makes
> > sense. The logic is simplified now (reset_sparsity is just passed along
> > to C++) as I don't see why it must be tied to the use of the cache. One
> > should be able to use the cache and reset the sparsity pattern.
> >
> >
> > ** Changed in: dolfin
> > Status: New => Fix Committed
> >
> > ** Changed in: dolfin
> > Assignee: (unassigned) => Anders Logg (logg)
>

[Johan Hake (johan-hake)]

On Tuesday September 20 2011 09:18:40 Anders Logg wrote:
> On Tue, Sep 20, 2011 at 03:42:22PM -0000, Johan Hake wrote:
> > Why do we use a cache in the first place? Wouldn't it be more natural
> > that a user keep track of any already assembled tensors.
>
> I don't know. I don't remember creating the cache. I thought you added
> it?

Nope.

From bzr annotate:

2668.1.42 logg@si | # Cache for tensors
                   | _tensor_cache = {}

Not sure we should remove it. It is pretty simple and transparent.

> > Also, my would a user want to use a cached Matrix _and_ reset the
> > sparsity?
>
> It may be stupid but it's up to the user. The old logic tried to
> impose some restrictions like not resetting the tensor when it was
> fetched from cache.

Ok.

Johan

> --
> Anders
>
> > Johan
> >
> > On Tuesday September 20 2011 07:17:05 Anders Logg wrote:
> > > I have made an attempt to fix this, but please check that it makes
> > > sense. The logic is simplified now (reset_sparsity is just passed along
> > > to C++) as I don't see why it must be tied to the use of the cache. One
> > > should be able to use the cache and reset the sparsity pattern.
> > >
> > >
> > > ** Changed in: dolfin
> > >
> > > Status: New => Fix Committed
> > >
> > > ** Changed in: dolfin
> > >
> > > Assignee: (unassigned) => Anders Logg (logg)