Move logintpack and libminifloat under DataFormats/Math, and add unit tests for logintpack::pack16 etc #22837
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22837/4225 |
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages: DataFormats/PatCandidates @perrotta, @monttj, @cmsbuild, @slava77, @gpetruc, @arizzi can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test |
|
The tests are being triggered in jenkins. |
|
I'm planning to use In practice |
|
On 4/4/18 5:34 AM, Matti Kortelainen wrote:
I'm planning to use |liblogintpack| for phase2 premixing (from package
under |Sim*|, @civanch <https://github.com/civanch>), so let me wonder
out loud already now if in light of that the |liblogintpack| and
|libminifloat| are still best placed in this |DataFormats/PatCandidates|
package? (I'm not claiming they should be moved, just asking beforehand
the opinions of people)
In practice |liblogintpack| is header-only, so |#include|ng it does not
create physical dependence, so maybe my pondering is more academical.
I think that the generic parts can easily be moved to DataFormats/Math
|
|
@makortel IIRC it is now possible to use the special "comment syntax" of Double32_t in our dict generation. < class name="myclass" / > < field name="_myDataMember" comment="hello" / > ... and this can be used with this comment syntax: The advantage would be that you do not need to do packing/unpacking in your class but it would be handled by root (on the other hand it introduces explicit dependency on root as a backend for this kind of data compression) |
|
@arizzi Interesting, thanks for the link. Following it, I'd interpret "x is converted to y bit unsigned integer" such that the mapping from floating point to integer is linear. But I believe in my case the logarithmic mapping of |
|
I think they also have some float truncation that is more similar to our reduced mantissa and liblogintpack, but I never really tested those because we couldn't use them when statred with miniaod. |
|
no objections at moving things to DataFormats/Math
…On Wed, Apr 4, 2018 at 3:29 PM, arizzi ***@***.***> wrote:
I think they also have some float truncation that is more similar to our
reduced mantissa and liblogintpack, but I never really tested those because
we couldn't use them when statred with miniaod.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22837 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEbbR9OWwGVwKvytjMjABvf61EiAr7Cyks5tlMqzgaJpZM4TGsRn>
.
|
|
@arizzi Thanks for point it out, reading the page carefully there is indeed "x converted to Float_t with truncated precision at 6 bits" and "if ... the double word will be converted to a float and its mantissa truncated to nbits significative bits". I'll try this option out as well (I'm currently packing the values inside a bitfield, so the outcome depends partly on how well ROOT manages to compress the "remaining" part of the bitfield). |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
makortel
changed the title
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22837/4245 |
|
The tests are being triggered in jenkins. |
|
Pull request #22837 was updated. @perrotta, @monttj, @vazzolini, @kmaeshima, @dmitrijus, @cmsbuild, @jfernan2, @slava77, @gpetruc, @vanbesien, @arizzi can you please check and sign again. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
@dmitrijus |
|
+1 |
|
btw, would it make sense to add those function to the dictionaries so that they are loadable in PyROOT/FWlite ? |
I don't disagree, but could that be left for a subsequent PR (by someone who knows what exactly should be put to |
|
+1 |
|
merge |
This PR adds unit tests for
logintpack::pack16()etc functions, in the case thatbaseis something smaller than 32768. I did these mainly to understand and make sure the 16-bit packing/unpacking work as I expected with smaller-than-15-bitbase.In addition, after some discussion below, the logintpack and libminifloat are moved under
DataFormats/Mathto be better usable outside PackedCandidates without adding a dependence onDataFormats/PatCandidates.Tested in CMSSW_10_1_0, no changes expected.