sxs-collaboration / sxs

Python code for manipulating data from the SXS collaboration
MIT License
25 stars 18 forks source link

Fix normalization bug in align2d script #75

Closed keefemitman closed 1 year ago

keefemitman commented 1 year ago

sxs's norm is defined as the sqrt of the L^2 norm, but in the align2d code we want the actual L^2 norm.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 66.66% and no project coverage change.

Comparison is base (b4c0a51) 42.29% compared to head (3483e09) 42.29%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #75 +/- ## ======================================= Coverage 42.29% 42.29% ======================================= Files 70 70 Lines 5578 5578 Branches 1109 1109 ======================================= Hits 2359 2359 Misses 3010 3010 Partials 209 209 ``` | [Impacted Files](https://app.codecov.io/gh/sxs-collaboration/sxs/pull/75?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sxs-collaboration) | Coverage Δ | | |---|---|---| | [sxs/waveforms/alignment.py](https://app.codecov.io/gh/sxs-collaboration/sxs/pull/75?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sxs-collaboration#diff-c3hzL3dhdmVmb3Jtcy9hbGlnbm1lbnQucHk=) | `9.09% <ø> (ø)` | | | [...veforms/rotating\_paired\_diff\_multishuffle\_bzip2.py](https://app.codecov.io/gh/sxs-collaboration/sxs/pull/75?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sxs-collaboration#diff-c3hzL3dhdmVmb3Jtcy9yb3RhdGluZ19wYWlyZWRfZGlmZl9tdWx0aXNodWZmbGVfYnppcDIucHk=) | `52.56% <66.66%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

moble commented 1 year ago

I think I agree with the actual code change, but not with your comment above.

First, the normalization should be the sum of squares (without a square-root) because it divides something that is explicitly constructed as the sum of squares. So I agree that they should be on the same footing, and your change to the code is correct. But maybe I'm missing something because I believe that WaveformModes.norm is defined as the $L^2$ norm, not its square-root. Maybe the documentation of WaveformModes.norm is poorly written?

I'll walk through my reasoning carefully, for myself as much as anything. There are some overlapping meanings for the notation $L^2$, but generally they all include squaring, then summing/integrating, and then taking a square-root. There is an annoying ambiguity with the term "norm" in the context of quaternions (and other composition algebras), where it might be more specifically called the "Cayley norm", and there is no square root. But AFAICT otherwise, a 2-"norm" always involve a square-root. In particular, the underlying concept here is the space of square-integrable functions, for which the $L^2$ norm is universally defined as

$$ \|f\|_2 := \left( \int |f|^2 d\mu \right)^{1/2} $$

We can expand $f$ in SWSHs, and then use orthonormality to find

$$ \begin{align} \|f\|2 &= \left( \int \left| \sum_{\ell,m} f_{\ell,m} \, {}_sY_{\ell,m} \right|^2 d\Omega \right)^{1/2} \ &= \left( \int \left( \sum_{\ell,m} f_{\ell,m} \, {}_sY_{\ell,m} \right)\left( \sum_{\ell',m'} \bar{f}_{\ell',m'} \, {}_s\bar{Y}_{\ell',m'} \right) d\Omega \right)^{1/2} \ &= \left( \sum_{\ell,m,\ell',m'} f_{\ell,m} \bar{f}_{\ell',m'} \int {}_sY_{\ell,m} \, {}_s\bar{Y}_{\ell',m'} d\Omega \right)^{1/2} \ &= \left( \sum_{\ell,m,\ell',m'} f_{\ell,m} \bar{f}_{\ell',m'} \delta_{\ell,\ell'} \delta_{m,m'} \right)^{1/2} \ &= \left( \sum_{\ell,m} f_{\ell,m} \bar{f}_{\ell,m} \right)^{1/2} \ &= \left( \sum_{\ell,m} \left| f\{\ell,m} \right|^2 \right)^{1/2} \end{align} $$

This last quantity is what np.linalg.norm returns, which is what WaveformModes.norm calls.

Is this consistent with your understanding of the code and/or documentation?

keefemitman commented 1 year ago

@moble Yeah my apologies for using sloppy wording. I think the confusion mainly arises because sxs.WaveformModes.norm is defined as $||f||_{2}$, i.e., the $L^{2}$ norm, as you've written out explicitly.

scri.WaveformModes.norm, however, is defined as $(||f||_{2})^{2}$, since the default value of take_sqrt is take_sqrt=False.

It would be nice if the two were consistent.

moble commented 1 year ago

Ah, okay. I forgot about that take_sqrt argument. That was an ugly choice, alright...

I wouldn't be opposed to making a breaking release of scri to change that, but I'm not sure if anyone uses that in existing code. There are a few uses within scri that would have to be changed. Thoughts?

For now, anyway, I'll just merge this. Thanks.

keefemitman commented 1 year ago

Yeah I agree that it's too big of a change (and not important enough of a change) to make to scri. Thanks for merging!