Skip to content
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

feat: adding DD4hep digitization, fix measurement writer, add variance in o… #785

Conversation

asalzburger
Copy link
Contributor

This PR:

  • adds a DD4hep digitization example for debugging digitization issue
  • fixes one problem of clusters not being present when running smearing only
  • adds the variance to the RootMeasurementWriter in order to be able to do pulls/variance checks

odd-residual

odd-pull

@asalzburger asalzburger added this to the next milestone Apr 27, 2021
@asalzburger asalzburger added Component - Examples Affects the Examples module Improvement Changes to an existing feature labels Apr 27, 2021
@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #785 (cea2fec) into master (87bb6e0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #785   +/-   ##
=======================================
  Coverage   48.67%   48.67%           
=======================================
  Files         328      328           
  Lines       16889    16889           
  Branches     7930     7930           
=======================================
  Hits         8220     8220           
  Misses       3072     3072           
  Partials     5597     5597           

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 87bb6e0...cea2fec. Read the comment docs.

Copy link
Contributor

@noemina noemina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are fine. Approving.

@@ -209,6 +211,19 @@ class RootMeasurementWriter final : public WriterT<MeasurementContainer> {
recBound[Acts::eBoundPhi] = fullVect[Acts::eBoundPhi];
recBound[Acts::eBoundTheta] = fullVect[Acts::eBoundTheta];
recBound[Acts::eBoundTime] = fullVect[Acts::eBoundTime];

Acts::BoundSymMatrix fullVar =
m.expander() * m.covariance() * m.expander().transpose();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my understanding: Should the second transform not just be the projection?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ja, but it operates on a matrix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I just thought that .expander.transpose was be the same as .projection, with the latter being preferred

@asalzburger asalzburger merged commit ec14282 into acts-project:master Apr 28, 2021
Copy link
Contributor

@niermann999 niermann999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved

@paulgessinger paulgessinger modified the milestones: next, v8.1.0 Apr 28, 2021
gagnonlg pushed a commit to gagnonlg/acts that referenced this pull request May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Examples Affects the Examples module Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants