search

TuringLang / AbstractMCMC.jl

Abstract types and interfaces for Markov chain Monte Carlo methods
https://turinglang.org/AbstractMCMC.jl
MIT License
87 stars 18 forks source link

Use BangBang to widen containers if needed #35

Closed devmotion closed 4 years ago

devmotion commented 4 years ago

This PR addresses the problem with transition_save! for stochastic control flow mentioned in https://github.com/TuringLang/DynamicPPL.jl/pull/105#issue-413520535. It changes the default implementations for writing and appending to a container of transitions from setindex! and push! to BangBang.setindex!! and BangBang.push!! which widen the container or return a new immutable instance if needed. For consistency with these default implementations and to indicate that one has to return the (potentially new) container, I renamed transitions_save! to `save!! (and transitions_init to just transitions).

I'm wondering, should we start with merging such breaking changes to a dev branch first? I assume that, e.g., solving or addressing https://github.com/TuringLang/AbstractMCMC.jl/issues/31 will also require some changes of the interface, and hence maybe it would make sense to gather multiple breaking changes before merging them into master and releasing them?

codecov[bot] commented 4 years ago

Codecov Report

Merging #35 into master will decrease coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
- Coverage   97.45%   97.43%   -0.03%     
==========================================
  Files           5        5              
  Lines         118      117       -1     
==========================================
- Hits          115      114       -1     
  Misses          3        3
Impacted Files Coverage Δ
src/AbstractMCMC.jl 100.00% <ø> (ø)
src/interface.jl 90.90% <100.00%> (-1.40%) :arrow_down:
src/sample.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c92a866...5c98582. Read the comment docs.

cpfiffer commented 4 years ago

I think we're fine to go into master in this repo, since AbstractMCMC doesn't change much and isn't very prone to bugs.

mohamed82008 commented 4 years ago

Is this good to go?