feat: update csv writer for digitization #694
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asalzburger
added
Component - Examples
|
Sorry @XiaocongAi for the many PR assignments, but his one is basically the same just with CSV instead of ROOT. |
Codecov Report
@@ Coverage Diff @@
## master #694 +/- ##
==========================================
- Coverage 49.03% 49.01% -0.02%
==========================================
Files 331 325 -6
Lines 16549 16574 +25
Branches 7725 7744 +19
==========================================
+ Hits 8115 8124 +9
- Misses 3013 3019 +6
- Partials 5421 5431 +10
Continue to review full report at Codecov.
|
|
Hi @XiaocongAi - if you have some time, this one should not take long, it's practically doing the same as the ROOT one from before. |
This PR moves all method definitions into the corresponding .cpp files. In local testing, I don't observe runtime performance degradation and slight compile-time improvement. Let's see what the CI has to say on the latter topic.
XiaocongAi
reviewed
Feb 9, 2021
There was a problem hiding this comment.
Hi @asalzburger , it's great to see this measurement writer. There are a few points I'm not sure about.
| /// Where to place output files | ||
| std::string outputDir; | ||
| /// Write the optional measurement surfaces collection | ||
| bool writeMeasurementSurfaces = false; |
There was a problem hiding this comment.
Is this writeMeasurementSurfaces playing a role somewhere?
There was a problem hiding this comment.
I guess we are not writing out the MeasurementParticlesMap because it could be derived from the MeasurementSimHitsMap, right?
| ActsExamples::ProcessCode ActsExamples::CsvMeasurementWriter::writeT( | ||
| const AlgorithmContext& ctx, const MeasurementContainer& measurements) { | ||
| const auto& simHits = ctx.eventStore.get<SimHitContainer>(m_cfg.inputSimHits); | ||
| // const auto& hitSimHitsMap = ctx.eventStore.get<IndexMultimap<Index>>( |
There was a problem hiding this comment.
It seems that the hitSimHitsMap has not been written out yet?
* Added a function adjustBinUtility() for rectangle bounds to BinAdjustment.hpp and modified BinUtility adjustBinUtility(const BinUtility& bu, const Surface& surface) accordingly. This is to make material mapping on renctangle layers possible. Modified the envelope from 1 mm to 0 mm in the TGeoLayerBuilder.hpp (otherwise I get a Radialbounds error when executing ActsExampleGeometryTGeo). * added a check to the plane surface in adjustBinUtility(const BinUtility& bu, const Surface& surface) for rectangular bounds. * added a unit test for the BinAdjustment of the rectangular plane. also split the if that checks for a plane surface and rectangular bounds in two. * changed the envelope of r and z in the TGeoLayerBuilder back to 1 mm. * commit after running ./CI/check_format_local Co-authored-by: Bernadette Kolbinger <bernadette.kolbinger@cern.ch> Co-authored-by: Paul Gessinger <paul.gessinger@cern.ch>
Clean up the magnetic field types and options in the examples: Magnetic field related headers are now consistently located in the ActsExamples/MagneticField include directory and all code is in the ActsExamples namespace. Before, the code still had the old Examples/Plugins/... layout and inconsistent or missing namespacing inherited from the old framework. Move the magnetic field variant type ActsExamples::MagneticField into a separate header. Magnetic field options have a well-documented order of preferrence (constant field overrides field map). If no field options are given, a null field is created, e.g. to support telescope-like detectors without a field. Expected units are clearly communicated as part of the parameter name, similar to what is done in other algorithms. The field map readers are private implementation details and are not publicly available anymore. Detector construction and magnetic field setup are now completely orthogonal: all magnetic fields can be setup for all detectors.
Co-authored-by: Andreas Salzburger <Andreas.Salzburger@cern.ch>
update licence statement Co-authored-by: Xiaocong Ai <xiaocong.ai@cern.ch>
update licence statement Co-authored-by: Xiaocong Ai <xiaocong.ai@cern.ch>
|
Will be superseded by #711 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a mirror PR to #692, but updates the CSV writing infrastructure.
It breaks backward compatibility of the TrackML library, which will anyway have to adapted to the new digitization capabilities.
It does not yet remove the
PlanarClusterinfrastructure, which will be done in a follow-up PR.