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

cmdstanpy 1.0.2 potentially logging warnings too often? #584

Open
tcuongd opened this issue Jun 29, 2022 · 5 comments
Open

cmdstanpy 1.0.2 potentially logging warnings too often? #584

tcuongd opened this issue Jun 29, 2022 · 5 comments

Comments

@tcuongd
Copy link

tcuongd commented Jun 29, 2022

Summary

I'm wondering if this change in 1.0.2:

CmdStanPy now computes some diagnostics after running HMC and will warn you about post-warmup divergences and treedepth exceptions

leads to unnecessary warnings.

Description:

After running an MCMC, this is the output of .diagnose():

Processing csv files: /var/folders/3x/ypx818511wsdnxc3knn7yy1r0000gn/T/tmpj9urc8se/prophet_modela2q2ppg4/prophet_model-20220629231921_1.csv, /var/folders/3x/ypx818511wsdnxc3knn7yy1r0000gn/T/tmpj9urc8se/prophet_modela2q2ppg4/prophet_model-20220629231921_2.csv, /var/folders/3x/ypx818511wsdnxc3knn7yy1r0000gn/T/tmpj9urc8se/prophet_modela2q2ppg4/prophet_model-20220629231921_3.csv, /var/folders/3x/ypx818511wsdnxc3knn7yy1r0000gn/T/tmpj9urc8se/prophet_modela2q2ppg4/prophet_model-20220629231921_4.csv

Checking sampler transitions treedepth.
21 of 600 (3.5%) transitions hit the maximum treedepth limit of 10, or 2^10 leapfrog steps.
Trajectories that are prematurely terminated due to this limit will result in slow exploration.
For optimal performance, increase this limit.

Checking sampler transitions for divergences.
No divergent transitions found.

Checking E-BFMI - sampler transitions HMC potential energy.
E-BFMI satisfactory for all transitions.

Effective sample size satisfactory.

Split R-hat values satisfactory all parameters.

Processing complete.

Overall it seems like the sampling was successful. However:

  1. cmdstanpy logged a warning at the end of the sampling:
23:23:47 - cmdstanpy - WARNING - Some chains may have failed to converge.
	Chain 1 had 5 iterations at max treedepth (3.3%)
	Chain 2 had 1 iterations at max treedepth (0.7%)
	Chain 3 had 13 iterations at max treedepth (8.7%)
	Chain 4 had 2 iterations at max treedepth (1.3%)

I'm wondering what the thresholds are for logging the non-convergence warning and whether they're too loose?

  1. I also get these warnings, although they might be specific to the model we're running with Prophet:
23:23:47 - cmdstanpy - WARNING - Non-fatal error during sampling:
Exception: normal_id_glm_lpdf: Scale vector is 0, but must be positive finite! (in '/Users/runner/work/prophet/prophet/python/stan/prophet.stan', line 137, column 2 to line 142, column 4)
Exception: normal_id_glm_lpdf: Scale vector is 0, but must be positive finite! (in '/Users/runner/work/prophet/prophet/python/stan/prophet.stan', line 137, column 2 to line 142, column 4)
Exception: normal_id_glm_lpdf: Matrix of independent variables is inf, but must be finite! (in '/Users/runner/work/prophet/prophet/python/stan/prophet.stan', line 137, column 2 to line 142, column 4)

It seems to be related to a type check of the stan program? I see this warning every time we do MCMC sampling now, even if overall the sampling procedure has no issues. Not sure where it's coming from -- ideally if it doesn't cause any sampling issues we wouldn't show it (in my opinion), but keen to get your thoughts.

In cmdstanpy==1.0.1, neither of these warnings were raised.

Thank you!!!

To reproduce, in case you need to:

from prophet import Prophet
import pandas as pd

df = pd.read_csv('https://raw.githubusercontent.com/tcuongd/prophet/tcuongd-covid-notebook/examples/example_pedestrians.csv')
m = Prophet(mcmc_samples=300)
m.fit(df, show_progress=False)

Current Version:

cmdstanpy==1.0.2

@WardBrian
Copy link
Member

I'm wondering what the thresholds are for logging the non-convergence warning and whether they're too loose?

These were introduced in #577. The current threshold is any of them being nonzero will lead to the warning you see

I also get these warnings, although they might be specific to the model we're running with Prophet

Yes, these are runtime warnings generated by Stan, unrelated to the above. Most likely they are happening for your model during adaptation and then they go away

Previously we only printed them if the Stan model exited with a nonzero exit code, however there are certain situations (mainly in the generate quantities block) where a fatal error still leads to a normal exit code. Without these warnings, it was impossible to know something went wrong unless you set show_console=True

@mitzimorris
Copy link
Member

mitzimorris commented Jun 29, 2022

agreed - warnings might be too strong for iterations at max treedepth.

the problem is this: we don't have enough information in the output to distinguish between whether or not NUTS hit the "U-turn" at that depth. that is, if you have an iteration at less than max-treedepth which doesn't hit a divergence, then you know that the sampler went as far as it could, but if you have an iteration at maxtreedepth, then you don't know what's going on, and the only way to figure that out would be to up the treedepth by 1 and see if the warnings go away - if that's the case, don't worry, but that's a very slow, inefficient way to do things.

thanks for bringing this up, and I'm totally open to suggestions as to what to do under the current regime.

this been discussed a number of times on Discourse and in various issues and meetings - e.g., https://discourse.mc-stan.org/t/divergence-vs-hitting-max-treedepth/9399

@WardBrian
Copy link
Member

We could set a configurable threshold for the treedepth and convergences if we didn't mind adding even more arguments to sample(). I believe cmdstanr has sample take a sequence of strings indicating which diagnostics you would like run, which is more binary

@mitzimorris
Copy link
Member

argument could be a dict - : diagnostics_thresholds { 'divergences' : int, 'max_treedepths': int } ?

@WardBrian
Copy link
Member

Revisiting this: it also seems reasonable to me to turn a lot of our calls to logging.warning into calls to the warnings.warn function, which would allow downstream users to much more easily filter them.

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

No branches or pull requests

3 participants