dstansby / demcmc

Differential Emission Measure estimation using MCMC methods
https://demcmc.readthedocs.io
BSD 3-Clause "New" or "Revised" License
7 stars 2 forks source link

Non-standard definition of density function for Gaussian distribution #67

Open matt-graham opened 1 year ago

matt-graham commented 1 year ago

In the documentation the expression given for the density of the Gaussian distribution assumed for the observation model

https://github.com/dstansby/demcmc/blob/a6d184a5625e5f468fce2639bac39f72c4d86e2b/doc/explanation/dem_theory.md?plain=1#L70

omits a factor of 2 in the denominator of the term inside the exponential compared to the usual definition of the probability density function for a Gaussian distribution - that is I think the more usual definition would be

$$ p \left (d{i} | \Theta \right ) = \exp - \left [ (I{i} - I{pred, i}(\Theta ))^{2} / (2 \sigma{i}^{2}) \right ] $$

This is also reflected in the implementation of the log probability density passed to emcee

https://github.com/dstansby/demcmc/blob/a6d184a5625e5f468fce2639bac39f72c4d86e2b/src/demcmc/mcmc.py#L29-L31

While this will still give a proper (normalizable) probability density function and so shouldn't cause problems when running MCMC, it does mean the $\sigma_i$ parameters no longer can be interpreted as the standard deviation of the observation noise (but are instead $\sqrt{2}$ times the standard deviation), which could be confusing. It might be this is just a different notational convention which is standard for this application, but if so it might be worth adding an explanatory note to the documentation to avoid possibility of confusing people like me who are more used to the $\sigma_i$ being the standard deviation!

dstansby commented 1 year ago

Thanks for pointing this out, and thanks for the clear explanation! Omitting the factor of two was not intentional, I shall label this as a bug that needs fixing.