search

CUQI-DTU / CUQIpy

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

Sampler rework via MCMC module (experimental module) #356

Closed chaozg closed 8 months ago

chaozg commented 10 months ago

Closes #365

I suggest looking at added demo file, the SamplerNew class and the added unit tests to get an idea of the changes first.

Issues to be raised but not resolved were added to #378

jakobsj commented 9 months ago

Many thanks for the update @nabriis, I believe we agreed that @chaozg would do the first review now. I did take a quick look and noticed that you had some nice "run test" scripts illustrating intended use that were removed in https://github.com/CUQI-DTU/CUQIpy/pull/356/commits/afbd63cb7fbb1b54abbc19dd24300b502372e66e. To speed up review it'd be incredibly helpful, at least for me, if you can reinclude these, for example put them, maybe merged into a single script, in the demos/ folder of this repo.

nabriis commented 9 months ago

@jakobsj Good suggestion. I re-wrote a demo script.

@chaozg I could not directly add you as reviewer. But please review :)

nabriis commented 8 months ago

Hello @chaozg and @jakobsj. Great points raised. I created issue #378 for most of the raised points. I did refactor into experimental module though. I would say this is ready for final approval

nabriis commented 8 months ago

Hi @amal-ghamdi. I added issue #379 for most of your specific comments. I think it is best we fix that specific thing in a seperate issue as I think it is a larger issue than one would think.