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

refactor: Refactor RootTrackStatesWriter #2648

Merged
merged 11 commits into from
Nov 13, 2023

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Nov 9, 2023

  • simplify code
  • remove magic -99 value
  • write non measurement states

@andiwand andiwand added this to the next milestone Nov 9, 2023
@github-actions github-actions bot added the Component - Examples Affects the Examples module label Nov 9, 2023
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a01f82a) 49.71% compared to head (42f24b3) 49.71%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2648   +/-   ##
=======================================
  Coverage   49.71%   49.71%           
=======================================
  Files         474      474           
  Lines       26859    26859           
  Branches    12362    12362           
=======================================
  Hits        13352    13352           
  Misses       4706     4706           
  Partials     8801     8801           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paulgessinger
Copy link
Member

I think there's an FPE in one of the physmon jobs. Would you be able to look into this @benjaminhuth?

Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

Could you do the change to momentum() from track state already here, then this would be complete ...

Examples/Io/Root/src/RootTrackStatesWriter.cpp Outdated Show resolved Hide resolved
@andiwand
Copy link
Contributor Author

I think there's an FPE in one of the physmon jobs. Would you be able to look into this @benjaminhuth?

did you mean to tag me? which FPE is this?

Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

Good to clear this up. Just a few comments, and a general question:

Don't we have to ensure that the vectors in the TTree have all the same lengths, so that everything stays consistent? Or is there some magic going on in ROOT?

Examples/Io/Root/src/RootTrackStatesWriter.cpp Outdated Show resolved Hide resolved
Examples/Io/Root/src/RootTrackStatesWriter.cpp Outdated Show resolved Hide resolved
benjaminhuth
benjaminhuth previously approved these changes Nov 13, 2023
Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

Looks good, only one stylistic thing came to my mind, but feel free to ignore it!

Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

Looks good!

@kodiakhq kodiakhq bot merged commit cc29c08 into acts-project:main Nov 13, 2023
55 checks passed
@andiwand andiwand deleted the refactor-root-trackstates-writer branch November 13, 2023 22:51
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Nov 13, 2023
@paulgessinger paulgessinger modified the milestones: next, v31.0.0 Nov 15, 2023
@paulgessinger paulgessinger removed the Fails Athena tests This PR causes a failure in the Athena tests label Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants