Skip to content

Fix mistake in SHO parameter C_0#11

Merged
tovrstra merged 6 commits intomolmod:acid1from
tovrstra:fix-acid1-descriptions
Apr 1, 2026
Merged

Fix mistake in SHO parameter C_0#11
tovrstra merged 6 commits intomolmod:acid1from
tovrstra:fix-acid1-descriptions

Conversation

@tovrstra
Copy link
Copy Markdown
Member

No description provided.

@tovrstra tovrstra requested a review from RobbeBohy March 27, 2026 10:40
Copy link
Copy Markdown
Collaborator

@RobbeBohy RobbeBohy left a comment

Choose a reason for hiding this comment

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

The definition Foreman-Mackey et al. use for the PSD is:

$$\sqrt{\frac{2}{\pi}} \frac{S_0 \omega_0^4}{(\omega^2 - \omega_0^2)^2 + \frac{\omega_0^2 \omega^2}{Q^2}}$$

If we plug in $\omega = 2 \pi f$ and the new definition $S_0 = \frac{C_0}{2}$, we still have a different prefactor as opposed to what is suggested by the ACID 1 documentation:

Now:

$$\sqrt{\frac{1}{2 \pi}} \frac{C_0 f_0^4}{(f^2 - f_0^2)^2 + \frac{f_0^2 f^2}{Q^2}}$$

ACID 1:

$$\frac{C_0 f_0^4}{(f^2 - f_0^2)^2 + \frac{f_0^2 f^2}{Q^2}}$$

Or is this difference exactly what is meant with the "unitary normalization convention for the Fourier transform"?

Comment thread acid-dataset/acid-dataset.typ Outdated
@RobbeBohy
Copy link
Copy Markdown
Collaborator

I just noticed an additional detail: the current version for the SHO ACF still uses $\tau$ instead of $\Delta_t$ for the $Q = \frac{1}{2}$ case.

@tovrstra
Copy link
Copy Markdown
Member Author

tovrstra commented Mar 27, 2026

There are some other differences too, where we should clarify things. The notation in ACID is such that C_0 is the integral of the autocorrelation function, which is convenient, but it deviates a bit from the convetions of the Lorentz model in STACIE. I'm fine with the difference, but it can be confusing. A more distinctive notation would help: C_0 here versus C_1 in STACIE is probably too similar and creates some expectations.

@tovrstra
Copy link
Copy Markdown
Member Author

Indeed, the 1/sqrt(2 pi) is due to the difference in normalization. The unitary convention of the continuous Fourier transform is relatively uncommon, and mainly adds more notation, so I'd rather avoid it.

@tovrstra
Copy link
Copy Markdown
Member Author

We could use $I_0$ instead of $C_0$. I refers to "integral", which is a bit similar to the STACIE papers and documentation, subcript zero hints at the zero frequency.

Another option is $\mathcal{I}$, but we never used this as a model parameter in our notation, so that's potentially confusing.

@RobbeBohy
Copy link
Copy Markdown
Collaborator

After thinking about this a bit more, perhaps $I_0$ could lead to confusion as well. In STACIE, the notation makes sense because we approximate the real $I$ via a model extrapolated to zero frequency, $I^{model} (f=0)$. For ACID, however, we never introduce such an approximation. We look directly at the zero‑frequency value of the kernel’s PSD, $C(f=0)$, rather than the zero‑frequency value of a model function.

For this reason, it seems least confusing to use the same symbol as the PSD but with a subscript 0. However, since our PSD is denoted $C(f)$, I understand the concern that this could be confused with the Lorentz $C_1$​. In that case, perhaps the most consistent solution would be to represent the PSD with a symbol other than $C$?

@tovrstra
Copy link
Copy Markdown
Member Author

That can be a useful change. I've currently used uppercase $C$ because it is the FT of lowercase $c$, but pairing symbols this way for Fourier transforms is not that critical.

The most common notation is $K_{xx}(t)$ (or $R_{xx}(t)$) and $S_{xx}(\omega)$. I slightly prefer an ordinary frequency for interpretative purposes in STACIE, so using $S$ could cause some confusion.

@RobbeBohy
Copy link
Copy Markdown
Collaborator

Between the two ACF notations, I would choose $K_{xx} (t)$ to avoid any possible confusion with position coordinates. I agree that using ordinary frequencies is clearer for STACIE, but wouldn’t this already be sufficiently explicit if we simply always write $S_{xx} (f)$ instead of $S_{xx} (\omega)$?

@tovrstra
Copy link
Copy Markdown
Member Author

I find it hard to come up with something that has no drawbacks.

Changing the notation of the ACF and PSD is a bit unpleasant because we have used the current notation already in published work. It would be better to keep the changes minimal, e.g. by only renaming the model parameters. I agree that $I_0$ is not ideal because the $I$ notation in the STACIE paper is for rescaled spectra, which is not relevant for the definitions of the kernels.

Something as mundane as a simple $A$ could work better. We could also take different symbols for the different models, e.g. $W$, $E$ and $H$ and give more descriptive names to the models themselves. We could still keep the zero subscript (smaller change, reference to zero frequency).

Comment thread acid-dataset/acid-dataset.typ Outdated
@tovrstra tovrstra requested a review from RobbeBohy April 1, 2026 12:01
Copy link
Copy Markdown
Collaborator

@RobbeBohy RobbeBohy left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

@tovrstra tovrstra merged commit f4ae9f8 into molmod:acid1 Apr 1, 2026
1 check passed
@tovrstra tovrstra deleted the fix-acid1-descriptions branch April 1, 2026 12:11
@tovrstra
Copy link
Copy Markdown
Member Author

tovrstra commented Apr 1, 2026

Thanks for checking. I'll proceed with preparing a 1.2.2 release on Zenodo, hopefully the last one in the 1.x series.

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.

2 participants