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: Allow external logger for ActsFatras/Simulator #728

Merged
merged 22 commits into from
Apr 7, 2021

Conversation

yifever
Copy link
Contributor

@yifever yifever commented Feb 26, 2021

Add a feature to use external loggers in ActsFatras. This feature is useful for running ActsFatras inside athena.

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #728 (c4c464a) into master (faf93e6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #728   +/-   ##
=======================================
  Coverage   48.90%   48.90%           
=======================================
  Files         325      325           
  Lines       16701    16701           
  Branches     7796     7796           
=======================================
  Hits         8168     8168           
  Misses       3062     3062           
  Partials     5471     5471           

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 faf93e6...c4c464a. Read the comment docs.

@paulgessinger paulgessinger added the 🚧 WIP Work-in-progress label Mar 1, 2021
@asalzburger
Copy link
Contributor

Hi @yifever - do you need some guidance/help on this to bet it going?

@FabianKlimpel will soon be in the position to merge the #574 in, then we could aim at a Fatras feature release.

@yifever yifever changed the title WIP: feat: Add new constructor in ActsFatras/Simulator that uses an external logger feat: Add new constructor in ActsFatras/Simulator that uses an external logger Mar 12, 2021
@yifever
Copy link
Contributor Author

yifever commented Mar 12, 2021

Hi @FabianKlimpel and @asalzburger
I took some time to fix all the unit test and formatting. This should be good now

@yifever
Copy link
Contributor Author

yifever commented Mar 15, 2021

I think I need someone to add a milestone for me. I am not a contributor and I am not sure I have that permission. (I have 0 experience in github and if there is a way for me to do this I apologize in advance for not finding the button.)

@asalzburger asalzburger added this to the next milestone Mar 17, 2021
@asalzburger
Copy link
Contributor

Ok, set the milestone and triggered merging of master into this, let's see.

@asalzburger asalzburger changed the title feat: Add new constructor in ActsFatras/Simulator that uses an external logger feat: Allow external logger for ActsFatras/Simulator Mar 17, 2021
@asalzburger asalzburger added Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Improvement Changes to an existing feature and removed 🚧 WIP Work-in-progress Component - Examples Affects the Examples module labels Mar 18, 2021
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.

Naming convention changes & some other small remarks.

Fatras/include/ActsFatras/Kernel/Simulation.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Kernel/Simulation.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Kernel/Simulation.hpp Outdated Show resolved Hide resolved
Fatras/include/ActsFatras/Kernel/Simulation.hpp Outdated Show resolved Hide resolved
@yifever
Copy link
Contributor Author

yifever commented Apr 2, 2021

Hi @asalzburger Can you look at the new changes?

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Let's get this in

@paulgessinger
Copy link
Member

I'm overriding the missing coverage results.

@paulgessinger paulgessinger merged commit d424ca1 into acts-project:master Apr 7, 2021
@paulgessinger paulgessinger modified the milestones: next, v7.0.0 Apr 8, 2021
FabianKlimpel pushed a commit to FabianKlimpel/acts that referenced this pull request Apr 29, 2021
Add a feature to use external loggers in ActsFatras. This feature is useful for running ActsFatras inside athena.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Fatras Affects the Fatras module Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants