Skip to content
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

quadrature comparison #255

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

quadrature comparison #255

wants to merge 3 commits into from

Conversation

ShawnL00
Copy link

No description provided.

Copy link
Collaborator

@alexfikl alexfikl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very cool! Just ran it locally to see the errors and there's a pretty big mismatch :\

Left some comments in there, but there's no big issues, just nitpicks. I imagine ruff is going to complain about some things, so might be worth running it locally until the CI recovers.

examples/planewave_quad_comparison.py Outdated Show resolved Hide resolved
examples/planewave_quad_comparison.py Outdated Show resolved Hide resolved
examples/planewave_quad_comparison.py Outdated Show resolved Hide resolved
examples/planewave_quad_comparison.py Outdated Show resolved Hide resolved
examples/planewave_quad_comparison.py Outdated Show resolved Hide resolved
examples/planewave_quad_comparison.py Outdated Show resolved Hide resolved
Copy link
Author

@ShawnL00 ShawnL00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the detailed feedback, Alex. I have run ruff, and it seems fine now.

@alexfikl
Copy link
Collaborator

alexfikl commented Jan 14, 2025

Thank you for the detailed feedback, Alex. I have run ruff, and it seems fine now.

That's great! Looks good to me now 😁

EDIT: Hm, no idea what those segmentation faults are about, but they don't seem related to this PR :\

@inducer
Copy link
Owner

inducer commented Jan 14, 2025

EDIT: Hm, no idea what those segmentation faults are about, but they don't seem related to this PR :\

Me either. What's weirder is that I tried to reproduce them in an interactive tmate session (#256), and no dice. Maybe conda-forge was temporarily in a weird state?

@inducer
Copy link
Owner

inducer commented Jan 14, 2025

It seems that jemalloc was at least involved in the problem. I've disabled it.

Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Some comments below.

Comment on lines +148 to +165
# VR quadrature
qbx_order = vr_target_order
sources_h, weights_nodes_h = quadrature(level,
vr_target_order, qbx_order=qbx_order)
vr_value = np.dot(wave(sources_h), weights_nodes_h)
vr_result.append(vr_value)
vr_nodes.append(len(sources_h[0]))
vr_err.append(np.abs(vr_value - ref))

# Tensor quadrature
qbx_order = tensor_target_order
sources_h, weights_nodes_h = quadrature(level,
tensor_target_order, qbx_order=qbx_order,
group_cls=TensorProductElementGroup)
tensor_value = np.dot(wave(sources_h), weights_nodes_h)
tensor_result.append(tensor_value)
tensor_nodes.append(len(sources_h[0]))
tensor_err.append(np.abs(tensor_value - ref))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a loop (since it's doing the same thing twice).

Comment on lines +61 to +73
n = Normalize[{-5, 4, 1}];
r = 1;
wave[θ_, φ_] :=
Exp[I r (n[[1]] Sin[θ] Cos[φ] +
n[[2]] Sin[θ] Sin[φ] +
n[[3]] Cos[θ])];

NIntegrate[
wave[θ, φ] * r^2 Sin[θ],
{θ, 0, Pi},
{φ, 0, 2 Pi},
WorkingPrecision -> 21
]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try to avoid using Mathematica? The main problem is that I can't reproduce the result, or tweak the computation in any way.

https://mpmath.org/doc/current/calculus/integration.html#mpmath.quad should work just as well.

actx = PyOpenCLArrayContext(queue)


def quadrature(level, target_order, qbx_order, group_cls=SimplexElementGroup):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def quadrature(level, target_order, qbx_order, group_cls=SimplexElementGroup):
def quadrature(level, target_order, qbx_order, group_cls):

Comment on lines +15 to +35
Order VR Exact_to Tensor Exact_to
-----------------------------------------
1 2 3
2 4 5
3 5 7
4 7 9
5 8 11
6 10 13
7 12 15
8 14 17
9 15 19
10 17 21
11 19 23
12 20 25
13 22 27
14 24 29
15 25 31
16 27 33
17 28 35
18 30 37
19 32 39
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not useful to incorporate this table, since it's not guaranteed to match the code on an ongoing basis. Instead, the code should generate the table.

actx = PyOpenCLArrayContext(queue)


def quadrature(level, target_order, qbx_order, group_cls=SimplexElementGroup):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way this is currently expressed is flawed: At no point do you explicitly specify which quadrature should be used. And what pytential uses internally is an implementation detail that could change at any time. If you're intending to measure differences between specific quadrature rules, your code should exert positive control over what rule is used, instead of relying on implementation coincidence. It may be easier to do this by using meshmode primitives.

qbx = QBXLayerPotentialSource(
pre_density_discr, target_order, qbx_order,
fmm_order=False)
discr_stage = sym.QBX_SOURCE_STAGE1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using pytential's stage-1 is a potential confounder, since it may apply refinement to the incoming discretization.


sources = density_discr.nodes()
weights_nodes = bind(places,
sym.weights_and_area_elements(ambient_dim=3, dim=2, dofdesc=dofdesc))(actx)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document how you convinced yourself that the error is not dominated by geometry derivatives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants