Skip to content
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

Add warning about change of sign in hessian functions #6312

Merged
merged 1 commit into from
May 9, 2024

Conversation

aseyboldt
Copy link
Member

@aseyboldt aseyboldt commented Nov 18, 2022

The hessian function incorrectly returned the negative of the hessian.

This does change the output of Model.d2logp however, so we should mention this in the release notes.

Major / Breaking Changes

Sign of the hessian is fixed, and therefore different than before

Fixes #6310

@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Merging #6312 (dfd1640) into main (77f24d7) will decrease coverage by 0.02%.
The diff coverage is 80.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6312      +/-   ##
==========================================
- Coverage   76.04%   76.03%   -0.02%     
==========================================
  Files          95       95              
  Lines       16181    16189       +8     
==========================================
+ Hits        12305    12309       +4     
- Misses       3876     3880       +4     
Impacted Files Coverage Δ
pymc/pytensorf.py 77.64% <66.66%> (-0.52%) ⬇️
pymc/model.py 71.46% <100.00%> (ø)
pymc/sampling/mcmc.py 89.88% <100.00%> (ø)
pymc/tuning/scaling.py 62.74% <100.00%> (ø)

@aseyboldt aseyboldt marked this pull request as draft November 18, 2022 15:37
@ricardoV94
Copy link
Member

ricardoV94 commented Nov 18, 2022

I opted to something like this when we changed the sign of an operation before:

pymc/pymc/math.py

Lines 308 to 331 in 4acd98e

def log1mexp(x, *, negative_input=False):
r"""Return log(1 - exp(-x)).
This function is numerically more stable than the naive approach.
For details, see
https://cran.r-project.org/web/packages/Rmpfr/vignettes/log1mexp-note.pdf
References
----------
.. [Machler2012] Martin Mächler (2012).
"Accurately computing `\log(1-\exp(- \mid a \mid))` Assessed by the Rmpfr package"
"""
if not negative_input:
warnings.warn(
"pymc.math.log1mexp will expect a negative input in a future "
"version of PyMC.\n To suppress this warning set `negative_input=True`",
FutureWarning,
stacklevel=2,
)
x = -x
return at.log1mexp(x)

If this is critical enough, we can do the same for the output of the hessian. And later on deprecate the kwarg

@michaelosthege michaelosthege added bug major Include in major changes release notes section labels Feb 5, 2023
@maresb
Copy link
Contributor

maresb commented Feb 5, 2023

Without understanding much context, as a geometer I must warn that a common sign convention for the Hessian in geometry and physics is indeed negative nabla-squared.

The reason for this is that it makes the Hessian positive semi-definite on plane-waves of the form $e^{i k x}$. Namely,

$$-\nabla^2 e^{i k x} = +k^2 e^{i k x}.$$

Nerd rant over. 🤓

@ricardoV94 ricardoV94 changed the title Fix aesaraf hessian function Add warning about change of sign in hessian functions Jun 16, 2023
@ricardoV94
Copy link
Member

@maresb I think that keeping it in line with PyTensor is a good reason enough?

@aseyboldt I added a FutureWarning that can be disabled with negate_output=False, and kept the default behavior the same. WDYT? Do you mind reviewing my changes? I also changed other places in the codebase to behave the same as before (expect a non-negated output and so on).

Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

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

Given that the typical user of PyMC isn't a differential geometer, I approve of this approach. I like @ricardoV94's deprecation plan

I'm still a bit 🤯 that this appears to be an accidental(?) rediscovery of the geometer's convention for the Laplacian. Although perhaps not too surprising, since when considering Bayesian stats as real-valued quantum mechanics, the minus sign that makes -logp positive-ish is the same minus sign that geometers like since it gives a positive spectrum.

@ricardoV94 ricardoV94 added maintenance and removed bug labels May 8, 2024
@ricardoV94 ricardoV94 marked this pull request as ready for review May 8, 2024 08:01
@codecov-commenter
Copy link

codecov-commenter commented May 8, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 76.03%. Comparing base (e42d35a) to head (dfd1640).

❗ Current head dfd1640 differs from pull request most recent head 374d47d. Consider uploading reports for the commit 374d47d to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #6312       +/-   ##
===========================================
- Coverage   91.24%   76.03%   -15.21%     
===========================================
  Files         102       95        -7     
  Lines       17090    16189      -901     
===========================================
- Hits        15593    12309     -3284     
- Misses       1497     3880     +2383     
Files Coverage Δ
pymc/model.py 71.46% <100.00%> (ø)
pymc/sampling/mcmc.py 89.88% <100.00%> (+2.14%) ⬆️
pymc/tuning/scaling.py 62.74% <100.00%> (ø)
pymc/pytensorf.py 77.64% <66.66%> (-13.58%) ⬇️

... and 87 files with indirect coverage changes

@ricardoV94 ricardoV94 merged commit 3729614 into pymc-devs:main May 9, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance major Include in major changes release notes section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hessian (d2logp) has the wrong sign
5 participants