CUQI-DTU / CUQIpy

https://cuqi-dtu.github.io/CUQIpy/
Apache License 2.0
44 stars 9 forks source link

Fix issue of invalid proposals being accepted #518

Closed chaozg closed 1 month ago

chaozg commented 1 month ago

fixed #494

With this PR, the following piece of code works now:

import cuqi
import matplotlib.pyplot as plt
import numpy as np

beta = cuqi.distribution.Beta(0.5, 0.5)
sampler = cuqi.experimental.mcmc.MH(beta, initial_point=np.array([0.1]), scale=0.1)
#sampler = cuqi.experimental.mcmc.ULA(beta, initial_point=np.array([0.1]), scale=0.1)
#sampler = cuqi.experimental.mcmc.MALA(beta, initial_point=np.array([0.1]), scale=0.1)
#sampler = cuqi.experimental.mcmc.NUTS(beta, initial_point=np.array([0.1]))
sampler.sample(10)
samples = sampler.get_samples()
samples.plot_trace()

MH, CWMH, ULA, MALA and NUTS are also tested for a "vector" Beta distribution with iid components

An irrelevant issue is observed and recorded at https://github.com/CUQI-DTU/CUQIpy/issues/519

chaozg commented 1 month ago

Thank @amal-ghamdi and @nabriis for your review.

@nabriis , I just added a test to check

I think I just take a simple (but naive) way to create a test function. Any suggestion to improve this test function is welcome : )

chaozg commented 1 month ago

Hey @chaozg. Thank you for fixing this issue. Could you somehow, based on the beta distribution design a test that runs through our list of samplers, sample e.g. 50 samples, then test if any invalid samples are present? This would catch the problem in future implementations. You can try to uncomment our isnan and isinf cases to check if the test would catch the issue.

Yes, just tested as you suggested: If we comment out the changes from this PR, the test can capture invalid samples, i.e., values not in the range of [0, 1] in this Beta case.

You can put the test in zexperimental/test_mcmc.py file. You can use one of the existing lists of samplers that we have as a variable in that file. Then try sampling the beta distribution for each of those samplers and compare and check for invalid.

It seems I can't use the existing list of samplers as I need to handpick only MH, ULA ,MALA, NUTS for bounded scalar distribution and only MH, CWMH, ULA ,MALA, NUTS for bounded vector distribution. But I might be very wrong here.

chaozg commented 1 month ago

Thank you @nabriis for the review and also the hint on using vercel black