search

probcomp / Gen.jl

A general-purpose probabilistic programming system with programmable inference
https://gen.dev
Apache License 2.0
1.79k stars 160 forks source link

add Base.copy(::ParticleFilterState) #509

Closed georgematheos closed 1 year ago

ztangent commented 1 year ago

Looks great! It strikes me that we might also want to implement Base.(:==) (and also Base.hash since that's required for every type that implements ==), so that we can check that copies are accurate? And to add a test case somewhere.

georgematheos commented 1 year ago

@ztangent For equality, what do you think between the following possible semantics?

For checking equality w.r.t. particle weights:

  1. Two PF states a and b are equal if a.log_weights .+ a.lml_estimate == b.log_weights .+ b.lml_estimate and all else is the same
  2. Two PF states a and b are equal if a.log_weights == b.log_weights && a.lml_estimate == b.lml_estimate and all else is the same

For dealing with new_traces: A. Two PF states which are identical except their new_traces field are equal. B. PF states must have equivalent new_traces fields to be equal.

The benefit of doing 1-A is that it would make it easier to do things like dictionary lookups based on equivalent PF states which might come from different paths through an inference algorithm. But it also seems like it could be confusing if, e.g., calling update_refs! on two equivalent PF states can yield non-equivalent states.

What do you think?

georgematheos commented 1 year ago

Re A vs B -- since we expect in normal usage the new_traces vector to have undef values, errors can be thrown if we try to check the equality or two new_traces arrays containing undef values. So I think we should potentially use semantics A.

ztangent commented 1 year ago

Thanks for raising these questions! I think 1-A sounds like a fine way to go -- or at least, I currently can't think of edge cases where you'd want a stricter equality check. Re 1, I'd maybe check that this is still correct when custom priority weights are used when resampling, as in the second branch of this code:

https://github.com/probcomp/GenParticleFilters.jl/blob/7eae3c1ff5036a48f6b02e88023a202f15c95e3c/src/resample.jl#L171-L184

Re A vs. B, I think not checking new_traces is fine given that it's not supposed to be exposed as part of the interface.

georgematheos commented 1 year ago

@ztangent I've committed a change which implements 2-A. I'm heading out on vacation now, so I will only be able to respond sporadically for the coming month, but hopefully this is a viable change for now!

Re implementation 1 vs 2 -- I switched to implementation 2. (Also, I realize that my proposal for "1" gets the math slightly wrong, by the way. This isn't the reason for the switch, though.)

The reason I switched from 1 to 2 is I realized I do not understand what the semantics are supposed to be regarding how the values of log_ml_est and log_weights are set. I had thought that the log_ml_est was just a performance optimization of some sort, and that in principle the ParticleFilterState data structure could be implemented where the log ML estimate was just obtained as logsumexp(log_weights) - n_particles, without changing any semantics. But as I'm looking at the code now, I realize that there are a couple methods that do not seem to treat the separation between log_weights and the log_ml_est as a semantically irrelevant implementation detail. get_log_weights can return different things, depending how much of the log ML estimate is factored into the weight vector and how much into log_ml_est. Likewise, the code you linked above appears like it might care about the separation of log_weights and log_ml_est, though I would need to read more carefully to be sure.

You or @alex-lew may understand these semantics, and so know if there is a better implementation of equality for the ParticleFilterState. Or perhaps it is actually the case that the semantics should always that the separation of log_ml_est and log_weights is irrelevant, and there are a couple of interface methods that currently mistakenly return different things depending on this separation?

Anyway, if this is going to require longer discussion, perhaps we could commit this implementation of 2-A for now, and open an issue to discuss potentially changing equality semantics for particle filters.

ztangent commented 1 year ago

Going with 2-A sounds good to me! Yes, on second thought, we probably shouldn't say two particle filters are equal if get_log_weights returns different things. I've made one additional commit to ensure that the implementation of hash is consistent with the semantics of ==. But apart from that, I think we can merge this -- thanks for making these changes! :)