Skip to content

Apply Sum Factorization Again#571

Open
jowezarek wants to merge 1 commit intodevelfrom
fix_sf_issue
Open

Apply Sum Factorization Again#571
jowezarek wants to merge 1 commit intodevelfrom
fix_sf_issue

Conversation

@jowezarek
Copy link
Contributor

Issue

The sum factorization (SF) algorithm is currently implemented for 3D bilinear forms defined on the interior of a domain (when not using openmp).
Psydac decides whether to use the SF algorithm by checking the dimension of the domain, whether openmp is used to parallelize the code, and by checking whether kernel_expr is an instance of sympde.expr.evaluation.DomainExpression.
The latter does not work in practice, as apparently in most cases (always?) kernel_expr is a tuple.

Fix

I changed this code snippet in api/discretization.py from

    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(kernel_expr, DomainExpression) # <- This check must be changed

        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)

to

    if isinstance(a, sym_BilinearForm):
        # Currently sum factorization can only be used for interior domains
        from sympde.expr.evaluation import DomainExpression
        if isinstance(kernel_expr, tuple):
            is_interior_expr = isinstance(kernel_expr[0], DomainExpression)
        else:
            is_interior_expr = isinstance(kernel_expr, 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)

@jowezarek jowezarek requested a review from yguclu February 3, 2026 15:22
Copy link
Member

@yguclu yguclu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch for finding this bug!

I have a couple of comments. Also, please update the file CHANGELOG.md.

Comment on lines +87 to +120
2026-02-03 09:53:14 (added by Julian O. - ThinkPad T14 in performance mode - after a suspicion that we don't use the newly added SF algorithm by default anymore)
----------

| Test case | old assembly | new assembly | old discretization | new discretization |
| --- | --- | --- | --- | --- |
| 2.1 | 25.953 | 27.642 | 17.806 | 9.641 |
| 2.2 | 2.818 | 3.324 | 6.66 | 3.463 |
| 2.3 | 48.497 | 49.256 | 22.308 | 9.821 |
| 3.1.1 | 0.428 | 0.476 | 2.183 | 1.238 |
| 3.1.2 | 0.983 | 1.336 | 2.323 | 1.2 |
| 3.1.3 | 0.848 | 0.786 | 4.028 | 2.252 |
| 3.2 | 1.676 | 1.369 | 1.513 | 1.662 |
| 3.3 | 0.949 | 0.586 | 1.327 | 1.288 |
| 3.4 | 0.694 | 0.923 | 1.144 | 1.161 |
| 3.5 | 0.612 | 0.558 | 1.564 | 1.511 |
| 3.6 | 1.865 | 0.624 | 2.379 | 1.76 |

2026-02-03 10:37:52 (added by Julian O. - ThinkPad T14 in performance mode - after fixing the above issue)
----------

| Test case | old assembly | new assembly | old discretization | new discretization |
| --- | --- | --- | --- | --- |
| 2.1 | 30.908 | 2.164 | 21.086 | 21.173 |
| 2.2 | 3.193 | 0.581 | 6.229 | 4.364 |
| 2.3 | 43.744 | 3.914 | 21.244 | 23.042 |
| 3.1.1 | 0.242 | 0.043 | 1.713 | 1.798 |
| 3.1.2 | 0.561 | 0.045 | 1.701 | 1.522 |
| 3.1.3 | 0.897 | 0.161 | 4.085 | 3.296 |
| 3.2 | 1.043 | 0.046 | 1.364 | 1.691 |
| 3.3 | 0.57 | 0.082 | 1.045 | 2.024 |
| 3.4 | 1.656 | 0.073 | 1.775 | 2.204 |
| 3.5 | 0.563 | 0.074 | 1.788 | 2.274 |
| 3.6 | 0.768 | 0.211 | 3.307 | 1.865 |

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that we should report all of these numbers. In particular, the timings seem to oscillate quite a bit. Maybe one should run these benchmarks a few times and report the average value?

Comment on lines +662 to +663
if isinstance(kernel_expr, tuple):
is_interior_expr = isinstance(kernel_expr[0], DomainExpression)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice, is kernel_expr anything else than a tuple? Could it be a list?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants