-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unify the definition of AbstractTraces
#14
Unify the definition of AbstractTraces
#14
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
This might affect #12 . The main change here is in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, butTrace
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
andMergedTraces
. - I think
MultiplexTraces
is a really nice addition. - I see the point of
MergedTraces
, it is like aTraces
but that can accommodate different types ofAbstractTraces
. I think in practice, because ofMultiplexTraces
, it is alwaysMergedTraces
that will be used. Theferefore I'm wondering ifTraces
should not become whatMergedTraces
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.
Sure, I'll add exhaustive tests to make sure the correctness for
Will do.
😄
(Done)
🤗
OK |
AbstractTraces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good !
Co-authored-by: Henri Dehaybe <47037088+HenriDeh@users.noreply.github.com>
Co-authored-by: Henri Dehaybe <47037088+HenriDeh@users.noreply.github.com>
No description provided.