Loosen tolerances for single precision - #260
Conversation
connorjward
left a comment
There was a problem hiding this comment.
This is slightly abstraction breaking. It would be nice for FIAT to not know about PETSc at all. I think the 'right' way to do this is to pass information about scalar type (accessible in the form_compiler_parameters) through to FIAT in the right places.
Would this be feasible? Seems like something AI could manage fairly well.
d845100 to
f292682
Compare
|
thanks @connorjward, what do you think? Now we are not using petsc4py but passing the scalar type from firedrake directly. |
connorjward
left a comment
There was a problem hiding this comment.
Looking pretty good now.
It would be good to have a couple very simple tests.
connorjward
left a comment
There was a problem hiding this comment.
I think I'm pretty happy at this point but I'm not comfortable merging without additional approval.
@rckirby thoughts?
connorjward
left a comment
There was a problem hiding this comment.
I think I am now happy, but as I said before this should get approval from someone else too.
|
This seems way too invasive, it seems that we only needed to deal with precision in FIAT due to a very niche case. |
|
I think a much nicer approach would be to communicate dtype through the reference cell coordinates |
|
FIAT elements are tabulated with the dtype given by the points array, so there is no need to be passing a redundant dtype kwarg |
pbrubeck
left a comment
There was a problem hiding this comment.
This PR does not need such a large diff
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
Co-authored-by: Pablo Brubeck <brubeck@protonmail.com>
Summary
Most
test_macro_grid_transfer/test_macro_multigridtests (macro elements: alfeld/iso split) fail under a single-precision PETSc build, but pass under double precision — mesh-hierarchy grid-transfer (inject/prolong/restrict) silently returns all-zero or NaN instead of the correct interpolated/projected value.Root cause:
compute_partition_of_unity's point-to-subcell binning tolerance (1e-12) is calibrated for double precision. A symbolic point's Newton-located reference coordinate has ~1e-6 error in single precision, causing points on a subcell boundary to bin to the wrong subcell and zero out locally-supported basis functions there.Needed for single-precision (fp32) support in firedrake, see firedrakeproject/firedrake#5033.
Changes
FIAT/precision.py:calibrate_tolerance(tol, dtype)— returnssqrt(tol)ifdtypeis single-precision (float32/complex64), elsetolunchanged. No dependency on PETSc/petsc4py.FIAT/reference_element.py:ufc_simplex,ufc_hypercube,ufc_cell, andsymmetric_simplexnow accept an optionaldtypekwarg, casting the constructed reference cell's vertices to that precision. This lets a caller (e.g. firedrake) declare the working precision when it builds a reference cell, rather than threading adtypeargument through the whole tabulation API.FIAT/expansions.py:ExpansionSet._tabulate's macro-element subcell-binning tolerance is now read directly fromself.ref_el.vertices.dtype, so it automatically reflects the precision the reference cell was built with.make_latticesilently upcasting to float64 (vianumpy.dotagainstrecursivenodes' always-float64 weights) even when given float32 vertices — this is what let a float32 macro-split complex still compute everything in float64.test/FIAT/unit/test_precision.py: parametrized unit tests forcalibrate_tolerance, plus two tests (using GEM, not sympy) verifying the tolerance relaxation actually reaches macro-element tabulation and changes classification near a subcell boundary.Discussion
Per review, this iterated from an earlier, much larger design (threading an explicit
dtypeparameter throughFiniteElement/PolynomialSet/all offinat) down to touching onlyreference_element.pyandexpansions.py— thedtypeonly ever mattered for this one macro-element tolerance, and the rest of the threading reached no real callers.This also resolves the earlier CI-coverage concern (FIAT's CI never installs petsc4py) — the current design has no PETSc dependency at all, so there's nothing precision-specific left uncovered by ordinary CI.
Still open, tracked in firedrake PR #5033: making firedrake's element-construction code actually pass
dtype=scalar_typewhen it builds reference cells, so this fix reaches real fp32 builds.AI disclosure: parts of this PR were developed with assistance from Claude (Anthropic). All changes have been reviewed, tested locally, and are understood by the author.