Add Strahlkorper power monitor #6642
Conversation
|
The macOS arm64 unit test failure |
knelli2
left a comment
There was a problem hiding this comment.
Looks good. You can squash in the dox changes.
Don't worry about the macOS tests. It's something unrelated that we haven't looked into yet.
src/NumericalAlgorithms/SphericalHarmonics/StrahlkorperFunctions.hpp
Outdated
Show resolved
Hide resolved
src/NumericalAlgorithms/SphericalHarmonics/StrahlkorperFunctions.hpp
Outdated
Show resolved
Hide resolved
e242666 to
356ee49
Compare
|
Thanks for the quick review, Kyle! Just squashed my docs changes, as you requested |
| template <typename Frame> | ||
| void power_monitor(gsl::not_null<DataVector*> result, | ||
| const Strahlkorper<Frame>& strahlkorper) { | ||
| *result = DataVector(strahlkorper.l_max() + 1, 0.0); |
| for (size_t l = 0; l <= strahlkorper.l_max(); ++l) { | ||
| const double normalization = | ||
| 2.0 * static_cast<double>( | ||
| l < strahlkorper.m_max() ? l : strahlkorper.m_max()) + |
| * \param strahlkorper The Strahlkorper surface. | ||
| */ | ||
| template <typename Frame> | ||
| DataVector power_monitor(const Strahlkorper<Frame>& strahlkorper); |
There was a problem hiding this comment.
Please return ModalVector. We should have the DG power monitors also do that. (not sure if we have bound ModalVector in Python yet)
There was a problem hiding this comment.
We should be consistent with what the DG power monitors do right now. So I'd say either leave this as DataVector and switch the power monitor return type in a separate PR, or add a separate commit that does the switch before adding this power monitor. I prefer the former as it's fairly unrelated to this PR.
There was a problem hiding this comment.
I used DataVector for consistency with AMR, so I could potentially reuse AMR functions. I'd rather change mesh and domain power monitors to ModalVector all at once in a separate PR
There was a problem hiding this comment.
I'm also concerned that ModalVector doesn't support as many math operations as DataVector. Are we sure that we even can change the power monitors for AMR to ModalVectors without just having to convert them back to do whatever operations we do on them? I think we should discuss on a future spectre call
There was a problem hiding this comment.
We can leave things as DataVector in this PR. It doesn't matter very much, so let's not worry about it too much.
Power monitors are modes, so they should be stored as ModalVector, so they are not confused as data on grid points (that's a DataVector). The math operations are only restricted to avoid the mixing between modal and nodal data.
356ee49 to
ab64994
Compare
|
Thanks for the review @nilsvu ... I pushed and squashed the two small changes you requested but (as I said above) would prefer to put switching this and the existing AMR power monitors to modal vectors in a different PR |
Proposed changes
Add a function in
StrahlkorperFunctions.cppthat takes a Strahlkorper and computes a power monitor, using the same method as SpEC does in SpEC'sSurfacePowerDiagnostics.cpp. This function is an ingredient for the "Shape" criteria for adaptive horizon finding. The existingpower_monitorfunctionality inNumericalAlgorithms/LinearOperators/PowerMonitors.?ppworks on meshes, but not on Strahlkorpers.Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments