-
Notifications
You must be signed in to change notification settings - Fork 282
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
Shorten, simplify and generalise code for Nédélec finite elements #3840
Conversation
6a268a4
to
ebbefa6
Compare
Job Coverage on b377c8b wanted to post the following: Coverage
Warnings
This comment will be updated on new commits. |
This is usually a good idea and a pretty easy one; see e.g. Heavily refactoring shape function definitions is also usually a good idea and usually not a pretty easy one; see e.g.
With Lagrange shape functions we cache results in between elements, but (unless the elements that are just shifts of each other, where we cache everything for everything) it doesn't quite save as much as you'd think; you don't have to recompute in master space each time but the physical space gradients have some cost.
|
6ac3a4d
to
d36c8ba
Compare
Perfect, my code was already very similar to that, but I've refactored to follow the same style.
Okay, let's leave that for later then. Let's focus on
Good, stored on the element then. |
0926a3f
to
55155d4
Compare
9d3baea
to
e5e9fa8
Compare
Sorry for the late review; this all looks like obvious improvements to me (assuming the higher- I'm going to be out shortly, but I should find time to merge this late today regardless unless anyone objects first. |
Hi Roy, Thanks a lot for finding the time to review this. There's two things I'd like to ask before you merge:
|
I'm fine with it, but I actually wouldn't have gone in that direction myself.
There are no families for which this couldn't happen, but there are two reasons why IMHO it shouldn't:
I didn't see any problems, but I fear my disdain for
When we see a non-conforming interface, we call
I'm afraid so. We have a generic |
These are exactly the two points I was worried about: 1) performance and 2) that we lose a potential check just like what you described (even though I couldn't find it). Before adding that commit, I actually checked myself that adding the element and node dofs produced the expression I was writing down under |
Perfect, so are we saying we can move this to
Hm, that's good. That means we can proceed without AMR support until we need it? 😇 |
We definitely still want to allow individual families to override the default, and we'll want to keep the existing per-family code for cases where there's a property that makes for simple+cheap reimplementation (e.g. LAGRANGE or SCALAR), or where there's a serious performance issue (e.g. RATIONAL_*), or where there's an issue in even defining a nodal solution sensibly (SIDE_HIERARCHIC) ... but yeah, I think at least 80% of our FE families probably ought to be calling the same code here. Let's save that for a separate PR, though.
Sadly, but yes. 😅 |
For completion and future reference. I compiled libMesh with MOOSE's default configuration plus BEFORE:
AFTER:
Same, but with BEFORE:
AFTER:
|
That's not a huge difference but it is much more than I'd expected. Init might be 1% slower or that might be my imagination, but compute looks 15% slower? IMHO that's worth it for more general and simpler code, but it's at least debatable. |
Yeah.... (sigh)... a bit disappointing... comparing the fastest times, init is roughly 1% slower and compute is about 15% slower for the smaller grid size and 12% slower for the larger grid size I just added. I'm inclined to agree with you that it's worth it as, at least for this potato problem, assembly is <0.5% of the total time... |
Hi Roy (@roystgnr), Alex (@lindsayad),
The overarching idea here is to generalise (some of) the code for arbitrary order finite elements.
I'm aware this would be a mammoth task, but I thought we could start with some small steps to better grasp the range of changes that'd be needed.
Since we are working on adding support for 2nd order Nédélec elements of the first kind, I thought instead of just hard-coding the various properties (e.g. the no. of dofs) we could try to generalise them. In the same spirit, for the shape functions, since they are orientation-dependent, I tried to avoid writing them twice as we had done thus far because even though that's not a lot for 1st order, trust me, it is for 2nd order. Here, it'd also be interesting to experiment with obtaining the shape functions from the dof definitions, although this would (a bit more fundamentally) change the way we think about assembly (we'd need to store the shape functions a-priori for a given element type and order perhaps the first time we see such an element).
Lastly, I'd like to understand what
n_dofs_per_elem
is supposed to return, and in particular its relationship with the middle node on TRI7, QUAD9 or HEX27 elements. For 2nd order Nédélec, we do have some dofs that are associated with the element, so where should they go? The middle node, or the element?Cheers,
-Nuno