-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[RFC:2] Use ROOT lossy compression for P3 and position of reco::Track #41018
[RFC:2] Use ROOT lossy compression for P3 and position of reco::Track #41018
Conversation
- added classes used by ROOT when doing storage which specify lossy compression. - 3 momentum has mantissa truncated to 13 bits - covariance matrix has mantisa truncated to 10 bits - position is truncated to 24 bits in the range +-1100 - added iotypes rule to use new classes
please test |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41018/34540
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages:
@mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-299027/31176/summary.html Comparison SummarySummary:
|
@perrotta @rappoccio would you be OK to make a special build with this PR (similar to what was done for #39554) ? I don't see a particularly strong motivation to backport and make that build in CMSSW_12_6_0_pre3. I think that for validation we will be able to get the needed confirmation from re-HLT workflows on standard relvals and there will be no need to do a 4-step special relval. Is there an ORP on Mar 14? |
Yes, we can make this special build, I will take care of it. There will not be an ORP today. |
Build started here |
@slava77 the release is now available, see https://cms-talk.web.cern.ch/t/development-release-cmssw-13-1-0-pre1-lossycompression-now-available/21477/1 |
looking at 1TeV muon relvals with reHLT Similar with 100 GeV muons has even smaller difference https://tinyurl.com/2kx6cp4v The new (compared to #39554) is clearly much better , but for the final version I think that it still may be safer to add 1 more bit and have 14. |
this is mostly in view of slightly better tracking resolution in phase-2 (while the only DQM setup that works out of the box is done on phase-1). |
@@ -453,7 +454,7 @@ namespace reco { | |||
HitPattern hitPattern_; | |||
|
|||
/// perigee 5x5 covariance matrix | |||
float covariance_[covarianceSize]; | |||
Float16_t covariance_[covarianceSize]; //[0,0,10] |
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.
to reduce confusion, it may be worth to be explicit and add a comment that Float16_t
is a full float in memory and is streamed as char (8 bit) exponent and 12 active bits for mantissa
https://root.cern.ch/doc/master/TBufferFile_8cxx_source.html#l00590
The double32 is a bit less confusing
@AdrianoDee I'm not sure we have tested the covariance changes to the full extent. |
@Dr15Jones @makortel |
I believe so, yes. |
<ioread | ||
sourceClass="ROOT::Math::DisplacementVector3D<ROOT::Math::Cartesian3D<Double32_t>,ROOT::Math::DefaultCoordinateSystemTag>" | ||
targetClass="reco::storage::TrackMomentumStorage" | ||
version="[1-]" |
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.
what's the effect of this; does it mean that any math::XYZVector
will be mapped to reco::storage::TrackMomentumStorage
even outside of reco::Track
?
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.
That is not what it means. The source is what is in the file so this would be for an older version of the class which used the Vector. The targetClass is the type within the data product in memory.
What causes the Vector class to be written out as the new type is the explicit declaration later in the file of
<field name="vertex_" iotype="reco::storage::TrackPositionStorage" />
<field name="momentum_" iotype="reco::storage::TrackMomentumStorage" />
as that is part of reco::TrackBase
declaration it will only happen to that class.
If the "lossy-compression" you are talking about is
|
for storage one my try to evaluate unums https://en.wikipedia.org/wiki/Unum_(number_format) |
I have tested the posit implementation in https://github.com/stillwater-sc/universal |
what's the "cm" range here? I think that we need around 1 µm precision at 10-30 cm. Just to be sure, by precision do you mean |
Overall, I still think that a configurable solution is really what's needed. so that we can write reduced precision in "commodity" tiers like AOD, while full precision can be done for precision tiers (RECO, ALCA). Otherwise this global level choice means that limited scope cases will drive the needs. |
with 20 bits one can have |
for one micron at 20cm one needs 21 bits |
Thanks. So, I guess at around 20-30 cm it will still be around 10 µm, which is likely going to be already visible on V0/conversion re-fits for vertices in O(10 cm) range. There is still a point on having ability to save states for the muon system; for the 10 m scale it still needs 10 µm. But perhaps this case should still be done with a TrackExtra providing the innermost state. Actually, even the V0-like cases are perhaps better served by ability to save innermost hit state where needed. For compression benefits saving uniform scale values e.g. with 0.1 µm precision (can be Another random thought: for vx,vy compression needs for tracking we would have a better compression by saving uniform range e.g. with 0.1 µm units relative to the beamspot. ... I still don't have a detailed understanding of the impact of the covariance rounding proposed in this PR. And this is the part with largest gain in storage. |
Here is a bit overdue summary of analysis related to the covariance rounding in phase-1 setup. The sim and reco steps took a bit of effort to make to get enough stats at high decay radius:
The plot is for the fitted V0/K_S vertex error in xy direction transverse to the K_S momentum, plotting the fraction of vertices with this transverse error below 30 µm. This captures the decays happening very close to the sensors.
Observations:
Recall that I guess these observations also suggest that related issues for displaced tracks may be present in miniAOD packing as well. @AdrianoDee are you aware of some detailed studies comparing miniAOD to AOD for displaced decay reconstruction in BPH context with cascades or conversions for decays with @pcanal @Dr15Jones would it be possible to expand the compression to cover 13-15 bit range for the significand precision for Float16_t and 15-17 for Some mitigation strategies for the displaced tracks will be needed if there is still a strong motivation to get the lossy compression enabled. One option could be to keep some skimmed (note in part to self: plan to make some slides to present this in e.g. TRK POG meeting) |
-1 |
Milestone for this pull request has been moved to CMSSW_14_0_X.Please open a backport if it should also go in to CMSSW_13_3_X. |
Milestone for this pull request has been moved to CMSSW_14_1_X. Please open a backport if it should also go in to CMSSW_14_0_X. |
@cmsbuild, please close Per #39449 (comment) |
cms-bot internal usage |
PR description:
PR validation:
Copied an AOD file from workflow 11834.21 into the new format. It reduced the file size by 13.5%. This compares to the previous PR (#39554) which on the exact same size reduced the file size by 13.9%.