forked from FEniCS/fiat
-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Construct differentiation matrix without sympy #47
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rckirby
reviewed
Oct 30, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support this endeavor and will review the code more carefully. A bit of FIAT history that is relevant to the discussion
- Originally, I had all the classic Duffy business and numerical dmats in FIAT, much as you are proposing. After writing a paper reworking the recurrence relations to avoid the corner singularity (https://dl.acm.org/doi/pdf/10.1145/1644001.1644006), I redid a lot of it, not yet symbolically.
- In a fit of laziness, I then used AD (but picked the Wrong Tool and created an awkward dependency) to get derivatives instead of using the derivative rules I worked out.
- To get around that (and for other reasons that @dham might recall), Somebody went through and stripped out the numerical dmat evaluation that was there (and you've reconstructed) in favor of sympy.
- I support going back to a numerical approach for speed reasons, but being able to get the basis functions as polynomials might be helpful, so making sure that the reworked evaluation rules let you put in x, y from sympy would be helpful.
-- Note: if you avoid coordinate singularities, the recurrence relations are obviously polynomials and don't rely on any cancellation. It's trivial for sympy to DTRT here, but it's not clear whether it would have problems cancelling Jacobians symbolically if you go through Duffy. - The code still seems to tabulate derivatives via
_tabulate_dpts
in sympy, but does the dmats numerically? I'm really afraid that we're going to make the whole thing very confusing. We need to decide what it should do and then make it do that.
…le improving high order accuracy
pbrubeck
force-pushed
the
pbrubeck/dmat-without-sympy
branch
from
November 9, 2023 22:30
56ce030
to
f628a01
Compare
This was referenced Nov 10, 2023
rckirby
approved these changes
Nov 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On simplex elements, if the degree is high enough, the main bottleneck is the construction of the differentiation matrix. We were doing this with sympy and that is extremely slow.
I replaced the tabulation of first and second derivatives with the derivative of the recurrence relation from (Kirby, 2010), and I am evaluating dmats lazily only if we ever need to tabulate derivatives.
This PR depends on #46
Here are the runtimes for a single call to
Lagrange(cell, degree)
before and after this PR:Before:
After: