-
Notifications
You must be signed in to change notification settings - Fork 191
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
Logical coordinates for spherical harmonic mesh #6389
base: develop
Are you sure you want to change the base?
Conversation
@@ -40,6 +40,8 @@ target_link_libraries( | |||
DataStructures | |||
ErrorHandling | |||
Options | |||
SPHEREPACK |
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.
Move spherepack to PRIVATE
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.
done
switch (mesh.basis(d)) { | ||
case Spectral::Basis::SphericalHarmonic: { | ||
switch (mesh.quadrature(d)) { | ||
case Spectral::Quadrature::Gauss: { |
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.
Add a comment and a note in the docs that the theta direction is identified by Gauss quadrature and the phi direction is identified by equiangular.
Also add notes that logical coordinates for the spherical harmonic basis will be already in spherical coordinates, not in [-1, 1], presumably because we never need the [-1, 1] coordinates and spherepack works directly with the collocation points in spherical coordinates.
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 basis functions are not defined on -1 to 1. It's only the standard Jacobi polynomials that are defined on this interval. There are basis functions defined on 0, 1
, 0, infty
, -infty, 0
, -infty, infty
, 0, pi
, etc. It's just that Jacobi polynomials happen to be orthonormal on [-1, 1]
, that's all. There's nothing generally special about those two numbers. This is just how basis functions work. I have nothing against stating the domain of the basis functions (we should do so for all of them, then!), but we shouldn't add any language implying there's something "weird" because they aren't on -1, 1
and that we magically "hid" the -1, 1
coordinates. That's not what's happening, there are no -1, 1
coordinates for all basis functions.
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.
Sounds good, there is a relation to Jacobi polynomials though since we use Legendre-Gauss points in cos(theta). Anyway, just stating the domain of the logical coords seems useful, and stating what collocation points are used for the ylm basis.
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 expanded the documentation.
// fall through | ||
case Spectral::Basis::FiniteDifference: { | ||
const auto& collocation_points_in_this_dim = | ||
Spectral::collocation_points(mesh.slice_through(d)); |
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.
Move the computation of the collocation points one level deeper if possible, into Spectral::collocation_points
. That way you also get the static caching for free.
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.
While it is possible, I don't think it makes sense to do so. There are currently ten functions that use the macro SPECTRAL_QUANTITY_FOR_MESH
and only collocation_points
would make sense with Basis::SphericalHarmonic. So since the branching is unique, why bury it and then require error handling for all the other functions? Furthermore collocation_points
calls compute_collocation_points_and_weights
to get the collcation points, and I don't think there are any useful weights
case Spectral::Basis::Chebyshev: | ||
// fall through | ||
case Spectral::Basis::Legendre: | ||
// fall through |
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.
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.
done
3149d10
to
194c768
Compare
Proposed changes
Computes the logical coordinates for a spherical harmonic basis
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments