-
Notifications
You must be signed in to change notification settings - Fork 51
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
Further improvements to lumped element geometry processing #248
Conversation
mfem::RefinedGeometry *RefG = refiner.Refine(geom, ref); | ||
T.Transform(RefG->RefPts, pointmat); |
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.
The points here, are they from the geometry or refinements of the interpolant? Basically, are they guaranteed to be on the geometry for any interpolating basis function.
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.
This isn't new code, but is the general "MFEM way" of extracting the nodes of a high-order curved element. I believe they will be on the geometry given the basis, but it probably depends on the basis and the conversion from the basis for the original mesh vs. the basis for high-order nodes in MFEM (I am fairly certain there is an interpolation step to convert the Gmsh equispaced basis to MFEM H1).
palace/fem/lumpedelement.cpp
Outdated
// For a non-planar element (shell of a rectangular prism, which can be "unfolded" to a | ||
// planar rectangle), compute the width as the perimeter of the bounding box orthogonal | ||
// to the length direction. | ||
w = 2.0 * std::accumulate(lengths.begin(), lengths.end(), 0.0); |
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.
What happens for an L shaped bracket of a port? I.e. an incompletely wrapping shell similar to the ones which triggered #89. This would give the sum of the two lengths rather than twice it.
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.
My understanding is the geometry in #89 was not L-shaped but was the entire shell of the rectangular prism (4 quad surfaces, not 2). But I do suppose an L-shaped port could also be valid and indeed would not be addressed by this. Maybe we have to fall back to the area calculation but that seems like a weird use case.
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.
From what I remember working on that issue (it was a while ago), the geometry was that L shape, which was what threw me off in debugging it, as I had assumed everything would be planar for lumped. That was the reason I stuck with the area calculation as opposed to pulling directly off the bounding box, then only if planar checked that it was a rectangle.
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.
Yep. To address this concern and also support #89, we'll take the following approach:
- Add a check for
bounding_box.planar
, like we do for coaxial lumped elements, and remove the second branch if the if/else which makes this 4-sided assumption. - For the case of Unclear Degenerate coordinates error #89, each side can be a separate element for a multielement lumped port, as:
"LumpedPort": [{ "Index": 1, "Elements": [{ "Attributes": [<attribute 1>], "Direction": "-Y" }, { "Attributes": [<attribute 2>], "Direction": "+Y" }, ... ]
This will give the correct behavior for the impedance BC, with each planar rectangular element adding in parallel much like a usual multielement lumped port.
I've added this in e4366f7 and claified the requirement in the docs in
8d24299.
A follow-up to #246, which removes the integration of the port area to calculate the geometry of uniform and coaxial lumped elements. This step was susceptible to discretization error, in particular for coaxial lumped ports with circular boundaries. These geometric properties can now be calculated directly using the element bounding box/sphere.
This change mandates rebaselining the coaxial regression tests. For the coaxial cable with inner and outer diameters of
1.6383
and5.461
mm, respectively, the impedance per square for the50
Ohm coaxial lumped port should be260.9
Ohm/sq (R = Rs * ln(R_o/R_i) / 2π
). The values computed by Palace and used internally for the impedance BC are:main
:261.8
Ohm/sq260.9
Ohm/sqThus, this PR corrects a previous geometry processing accuracy issue which showed up for coarsely meshed curves. Looking at the port V and I time histories, it looks like the errors were on the order of 1% for this example.