Unify the definition of AbstractTraces

[findmyway]

No description provided.

[codecov-commenter]

Codecov Report

Merging #14 (344b3b6) into main (c2db98d) will increase coverage by 50.66%.
The diff coverage is 78.35%.

@@             Coverage Diff             @@
##             main      #14       +/-   ##
===========================================
+ Coverage   30.06%   80.73%   +50.66%     
===========================================
  Files           9        8        -1     
  Lines         316      301       -15     
===========================================
+ Hits           95      243      +148     
+ Misses        221       58      -163     

Impacted Files	Coverage Δ
src/trajectory.jl	80.76% <0.00%> (-10.90%)	⬇️
src/traces.jl	83.91% <80.99%> (-1.80%)	⬇️
src/common/CircularArraySARTTraces.jl	100.00% <100.00%> (+100.00%)	⬆️
src/common/CircularArraySLARTTraces.jl	100.00% <100.00%> (+100.00%)	⬆️
src/patch.jl	100.00% <100.00%> (ø)
src/samplers.jl	100.00% <100.00%> (ø)
src/common/sum_tree.jl	66.21% <0.00%> (+66.21%)	⬆️

---

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 c2db98d...344b3b6. Read the comment docs.

[findmyway]

This might affect #12 .

The main change here is in the traces.jl file. I replaced the Trace with LastDimSlices. Generally speaking, each trace now can be any AbstractVector.

[HenriDeh]

I think these changes are not too problematic for #12.

• If I understand well the new Trace, the idea is that the container (parent) is, for example, a CircularArrayBuffer, but Trace make it look like a Vector and will return a view when fetched. I think an example in the docstring would help understand this better. I would also add unit tests that combine both because this will probably be the main use case.
• I'd do the same for Traces, MultiplexTraces and MergedTraces.
• I think MultiplexTraces is a really nice addition.
• I see the point of MergedTraces, it is like a Traces but that can accommodate different types of AbstractTraces. I think in practice, because of MultiplexTraces, it is always MergedTraces that will be used. Theferefore I'm wondering if Traces should not become what MergedTraces is at the moment.

There are plenty of changes in this PR, it's a bit hard to get a hang of the new design just by looking at the code. So I'll wait for this to be merged to check if #12 works well with it. But overall it seems nice. I may have new suggestions in the future after fiddling with this a bit longer.

I think a good addition right now would be unit tests this with non-scalar traces and with episodes. I think this may highlight potential caveats and also serve as a proto-documentation of examples.

[findmyway]

I think these changes are not too problematic for #12.

• If I understand well the new Trace, the idea is that the container (parent) is, for example, a CircularArrayBuffer, but Trace make it look like a Vector and will return a view when fetched. I think an example in the docstring would help understand this better. I would also add unit tests that combine both because this will probably be the main use case.

Sure, I'll add exhaustive tests to make sure the correctness for CircularArrayBuffers.

• I'd do the same for Traces, MultiplexTraces and MergedTraces.

Will do.

• I think MultiplexTraces is a really nice addition.

😄

• I see the point of MergedTraces, it is like a Traces but that can accommodate different types of AbstractTraces. I think in practice, because of MultiplexTraces, it is always MergedTraces that will be used. Theferefore I'm wondering if Traces should not become what MergedTraces is at the moment.

This is worth rethinking. I'll give it a try.

(Done)

There are plenty of changes in this PR, it's a bit hard to get a hang of the new design just by looking at the code. So I'll wait for this to be merged to check if #12 works well with it. But overall it seems nice. I may have new suggestions in the future after fiddling with this a bit longer.

🤗

I think a good addition right now would be unit tests this with non-scalar traces and with episodes. I think this may highlight potential caveats and also serve as a proto-documentation of examples.

OK

[HenriDeh]

Looks good !