(O2-1203) [CTF] use merging/splitting iterators during CTF ecoding/decoding for TPC #5199
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MichaelLettrich
requested review from
davidrohr,
sawenzel,
shahor02,
wiechula and
a team
as code owners
MichaelLettrich
force-pushed
the
O2-1203-rans-library
branch
from
1035af1
to
905161f
Compare
|
@MichaelLettrich thanks, I've tested it on 200 pbpb events TF, works fine with the same output size as before. It is marked as WIP, do you have other stuff to commit? |
|
@shahor02 thanks for testing. Feature complete but needs some cleanups in the git history. Opened it as WIP to see if formatting was OK. Will remove WIP and ask for review once git cleanup is done. |
* `CombinedInputIterator` has to be bidirectional since rANS encoder reads backwards. * `CombinedInputIterator` has to be default constructable. Dereferencing it is undefined behaviour though. * Adjust Encoders to work with bidirectional iterators
* `tpc::CTFCoder` uses `CombinedInputIterator` to avoid a temporary vector and elides 2 copies * `tpc::CTFCoder` uses lambdas and type_traits instead of macros to avoid incomprehensible compiler errors * `ctf::EncodedBlocks` remove deref of past-the-end-iterator which is undefined behaviour * Unit tests use merged columns add iterator TPC makeInputIterators replace makecoder macro replace ENCODETPC macro with lambda fix undefined behaviour
* `CombinedOutputIterator` needs to set `value_type` correctly to work with `iterator_traits`. This needs to be passed upon instantiation as a template parameter and cannot be auto-deduced. * `ComninedOutputIterator` fix bug with std::tie caused by returning `ComninedOutputIterator::Proxy` by value when dereferencing the iterator. * `CombinedOutputIteratorFactory` simplifies construction of `CombinedOutputIterator` instances.
* `EncodedBlocks` allow more general iterators in decoder methods * `CTFCoder` use `CombinedOutpurIterator` when decoding merged columns to elide a temporary vector and two copies per element. * `CTFCoder` use functions instead of preprocessor macros for decoding for better compiler error messages
* Use `std::enable_if` type trait for compile-time type checks if iterator holds correct value type
MichaelLettrich
force-pushed
the
O2-1203-rans-library
branch
from
bd1f13d
to
b955309
Compare
MichaelLettrich
changed the title
|
@shahor02. Should be good for review now. Can you have a look? |
shahor02
approved these changes
Jan 15, 2021
There was a problem hiding this comment.
@MichaelLettrich already did and tested, thanks. Will merge once tests are passed
|
@shahor02 thanks a lot 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
o2::tpc::CTFCoderto use merging/splitting iterators fromo2::rans::utils. This avoids storing intermediate vectors and removes two copy operations per merged/ splitted element and should visibly improve performance in CTF creation.EncodedBlocksto remove artificial restriction to pointers and use iterators insteadCombinedInputIteratorandCombinedOutputIteratorto make it usable in real world applications