Fix sum factorization

[yguclu]

Avoid failures in the new 3D assembly algorithm based on sum factorization:

• Do not use it on boundaries and interfaces
• Initialize self._func to None
• Handle complex quantities

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ -2.42%	✅ 46.67%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (689127d)	32041	19903	62.12%
Head commit (d852de0)	64102 (+32061)	38266 (+18363)	59.70% (-2.42%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#558)	15	7	46.67%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

[jowezarek]

Hey @yguclu, the implemented changes look good to me, I only have two comments:

1. In api/discretization.py::discretize, there's this piece of code:

      if isinstance(a, sym_BilinearForm):
          if kwargs.pop('sum_factorization'):
              return DiscreteBilinearForm_SF(a, kernel_expr, *args, **kwargs)
          else:
              return DiscreteBilinearForm(a, kernel_expr, *args, **kwargs)

   Don't you think we could, just like you've done in api/fem_sum_form.py, already check here whether a is an interior expression? Somethink along the lines of

      if isinstance(a, sym_BilinearForm):
          # Currently sum factorization can only be used for interior domains
          from sympde.expr.evaluation import DomainExpression
          is_interior_expr = isinstance(e, DomainExpression)
          if kwargs.pop('sum_factorization') and is_interior_expr:
              return DiscreteBilinearForm_SF(a, kernel_expr, *args, **kwargs)
          else:
              return DiscreteBilinearForm(a, kernel_expr, *args, **kwargs)

2. In api/fem_bilinear_form.py, parts of the code are apparently not tested. Between line 534 and line 655 we want to use StencilInterfaceMatrix, knot_insertion_projection_operator and ComposedLinearOperator multiple times, but these two classes and this one function are never imported.

[yguclu]

Hey @yguclu, the implemented changes look good to me, I only have two comments:

1. In api/discretization.py::discretize, there's this piece of code:

      if isinstance(a, sym_BilinearForm):
   
          if kwargs.pop('sum_factorization'):
   
              return DiscreteBilinearForm_SF(a, kernel_expr, *args, **kwargs)
   
          else:
   
              return DiscreteBilinearForm(a, kernel_expr, *args, **kwargs)

   Don't you think we could, just like you've done in api/fem_sum_form.py, already check here whether a is an interior expression? Somethink along the lines of

      if isinstance(a, sym_BilinearForm):
   
          # Currently sum factorization can only be used for interior domains
   
          from sympde.expr.evaluation import DomainExpression
   
          is_interior_expr = isinstance(e, DomainExpression)
   
          if kwargs.pop('sum_factorization') and is_interior_expr:
   
              return DiscreteBilinearForm_SF(a, kernel_expr, *args, **kwargs)
   
          else:
   
              return DiscreteBilinearForm(a, kernel_expr, *args, **kwargs)

This suggestion makes sense to me, but it should be tested! Could you please push a commit so we can check if it works?

2. In api/fem_bilinear_form.py, parts of the code are apparently not tested. Between line 534 and line 655 we want to use StencilInterfaceMatrix, knot_insertion_projection_operator and ComposedLinearOperator multiple times, but these two classes and this one function are never imported.

Good catch! What do you propose exactly?

[yguclu]

Hey @yguclu, the implemented changes look good to me, I only have two comments:

1. In api/discretization.py::discretize, there's this piece of code:

         if isinstance(a, sym_BilinearForm):
             if kwargs.pop('sum_factorization'):
                 return DiscreteBilinearForm_SF(a, kernel_expr, *args, **kwargs)
             else:
                 return DiscreteBilinearForm(a, kernel_expr, *args, **kwargs)

   Don't you think we could, just like you've done in api/fem_sum_form.py, already check here whether a is an interior expression? Somethink along the lines of

      if isinstance(a, sym_BilinearForm):
          # Currently sum factorization can only be used for interior domains
          from sympde.expr.evaluation import DomainExpression
          is_interior_expr = isinstance(e, DomainExpression)
          if kwargs.pop('sum_factorization') and is_interior_expr:
              return DiscreteBilinearForm_SF(a, kernel_expr, *args, **kwargs)
          else:
              return DiscreteBilinearForm(a, kernel_expr, *args, **kwargs)

I have done this in d852de0

[jowezarek]

Looks good to me now!