Conversation
ksagiyam
force-pushed
the
ksagiyam/tsfc_refactor_1
branch
7 times, most recently
from
5cff887 to
4d2358d
Compare
ksagiyam
force-pushed
the
ksagiyam/tsfc_refactor_1
branch
from
1280275 to
2395192
Compare
tsfc/driver.py
Outdated
| self.domain_number = domain_number | ||
| self.coefficients = coefficients | ||
| self.coefficient_numbers = coefficient_numbers | ||
| super(TSFCIntegralDataInfo, self).__init__() |
There was a problem hiding this comment.
If this is an immutable object does it make sense for it just to be a NamedTuple?
tsfc/kernel_interface/common.py
Outdated
| class KernelBuilderMixin(object): | ||
| """Mixin for KernelBuilder classes.""" | ||
|
|
||
| @cached_property |
| """Mixin for KernelBuilder classes.""" | ||
|
|
||
| @cached_property | ||
| def fem_config(self): |
tsfc/kernel_interface/firedrake.py
Outdated
| subdomain_id = integral_data_info.subdomain_id | ||
| domain_number = integral_data_info.domain_number | ||
| super(KernelBuilder, self).__init__(scalar_type, integral_type.startswith("interior_facet")) | ||
| self.fem_scalar_type = fem_scalar_type |
There was a problem hiding this comment.
Refactor so that parameters["scalar_type"] is the only thing that is passed in and the coffee builder constructs the C-name of the type internally.
There was a problem hiding this comment.
Then we don't need to pass two scalar types that are generally the same here
There was a problem hiding this comment.
Thanks! Fixed using coffee.base.as_cstr().
ksagiyam
force-pushed
the
ksagiyam/tsfc_refactor_1
branch
from
2395192 to
4793fc4
Compare
tsfc/kernel_interface/common.py
Outdated
| class KernelBuilderMixin(object): | ||
| """Mixin for KernelBuilder classes.""" | ||
|
|
||
| def compile_ufl(self, integrand, params, ctx): |
There was a problem hiding this comment.
Call this compile_integrand ?
tsfc/kernel_interface/common.py
Outdated
| """Compile UFL integrand. | ||
|
|
||
| :arg integrand: UFL integrand. | ||
| :arg params: a dict of parameters containing quadrature info. |
| self.argument_multiindices, | ||
| params) | ||
|
|
||
| def stash_integrals(self, reps, params, ctx): |
| for var, rep in zip(self.return_variables, reps): | ||
| mode_irs[mode].setdefault(var, []).append(rep) | ||
|
|
||
| def compile_gem(self, ctx): |
There was a problem hiding this comment.
Docstring please.
Can we also document (somewhere, maybe not here) the interface that ctx must offer?
There was a problem hiding this comment.
Added more texts under KernelBuilderMixin.create_context method.
| """ | ||
| def __init__(self, ast=None, integral_type=None, oriented=False, | ||
| subdomain_id=None, domain_number=None, quadrature_rule=None, | ||
| subdomain_id=None, domain_number=None, |
There was a problem hiding this comment.
The quadrature rule is used by themis: https://github.com/celdred/themis, maybe instead of dropping it replace this commit with a comment that that is what it is for.
There was a problem hiding this comment.
I wonder what themis is expecting from quad_rule. quad_rule is a local variable in for integral in integral_data.integrals loop (https://github.com/firedrakeproject/tsfc/blob/master/tsfc/driver.py#L193), but the last value it happens to take is passed to builder.construct_kernel(...) (https://github.com/firedrakeproject/tsfc/blob/master/tsfc/driver.py#L266), which themis seems to require.
ksagiyam
force-pushed
the
ksagiyam/tsfc_refactor_1
branch
from
4793fc4 to
fc66951
Compare
ksagiyam
force-pushed
the
ksagiyam/tsfc_refactor_1
branch
from
fc66951 to
c260ac8
Compare
ksagiyam
force-pushed
the
ksagiyam/tsfc_refactor_1
branch
from
c260ac8 to
4a1cd20
Compare
3 participants
Breaking #234 into smaller pieces (Phase 1)
This PR attempts to refactor TSFC.
Firedrake PR firedrakeproject/firedrake#2200 must follow this.