Project

General

Profile

Support #18619

Add charge information to recob::SpacePoint

Added by Tingjun Yang almost 2 years ago. Updated almost 2 years ago.

Status:
Closed
Priority:
Normal
Category:
Data products
Target version:
-
Start date:
12/21/2017
Due date:
% Done:

100%

Estimated time:
Spent time:
Experiment:
-
Co-Assignees:
Duration:

Description

Dear LArSoft experts,

I am wondering if we can add a variable to recob::SpacePoint to save charge information. This would address an immediate need for SpacePointSolver and WireCell reconstruction while waiting for the long-term solution.

Thanks,
Tingjun

Associated revisions

Revision c54892fa (diff)
Added by Gianluca Petrillo almost 2 years ago

Added proxy for space points associated with charge.

This is client part for solution of issue #18619.

Revision 4d39e287 (diff)
Added by Gianluca Petrillo almost 2 years ago

Adding recob::PointCharge data product class.

This is the main solution to issue #18619.

Revision f1fb4e16 (diff)
Added by Gianluca Petrillo almost 2 years ago

Introduced a helper to write space points and associated charge.

This is part of the client interface for solution of issue #18619.
(the commit also applies an class renaming)

Revision 1e7d9947 (diff)
Added by Gianluca Petrillo almost 2 years ago

Added dumper for space points with charge

Part of the infrastructure of issue #18619.

Revision 8626368f (diff)
Added by Gianluca Petrillo almost 2 years ago

SpacePointSolver now uses new charged space point infrastructure

This completes the solution of issue #18619.

History

#1 Updated by Gianluca Petrillo almost 2 years ago

  • Category set to Data products
  • Status changed from New to Assigned
  • Assignee set to Gianluca Petrillo

Could you point me to an existing producer which would be filling this information?

#2 Updated by Tingjun Yang almost 2 years ago

Chris Backhouse's SpacePointSolver module makes space points and can save the charge information. I have added Chris, which can point out where the charge information is.

#3 Updated by Christopher Backhouse almost 2 years ago

My impression was that everyone had basically agreed to this scheme:
https://indico.fnal.gov/event/15227/contribution/0
Did anything ever get implemented?

#4 Updated by Gianluca Petrillo almost 2 years ago

I had forgotten that presentation.
The implementation I am carrying on now includes only the addition of the charge, without the disincorporation of the space point fit information.

#5 Updated by Gianluca Petrillo almost 2 years ago

  • Status changed from Assigned to Feedback
  • % Done changed from 0 to 90

The proposed solution to the charge addition, following the guidelines that Chris has found in the talk, is in branch feature/gp_Issue18618 of lardataobj (data product), lardata (proxy, dumper, writer) and larreco (implementation in SpacePointSolver module).
As usual, the classes, libraries and tools come with Doxygen documentation and "unit" tests, but I did not test the changes in SpacePointSolver. I would appreciate if Christopher could give it a try. The ticket is in feedback waiting the blessing of Christopher.

recob::PointCharge data product class

The data product has the content illustrated in the presentation, but it follows LArSoft prescriptions (and it's 170 lines, not 4 any more).

proxy::ChargedSpacePoints proxy

The provided proxy allows the parallel navigation of space points and charge.
Examples of usage are the unit test (lardata:source:test/RecoBaseProxy/ChargedSpacePointProxyTest_module.cc) and the dumper (lardata:source:lardata/ArtDataHelper/Dumpers/DumpChargedSpacePoints_module.cc).

recob::ChargedSpacePointsCollectionCreator tool to create the data products

The long-named class recob::ChargedSpacePointsCollectionCreator facilitates the creation of space points and charges satisfying the prescriptions from the proxy.
Examples of usage are in the unit test of the proxy (lardata:source:test/RecoBaseProxy/ChargedSpacePointInputMaker_module.cc) and Christopher's module (larreco:source:larreco/SpacePointSolver/SpacePointSolver_module.cc).

DumpChargedSpacePoints dumper module

The module DumpChargedSpacePoints can dump on screen the data of space points and their associated charge.
A sample configuration file dump_chargedspacepoints.fcl is provided (default value, reco3d, has been chosen after the only SpacePointSolver configuration I know of, in ProtoDUNE).
Run lar --print-description DumpChargedSpacePoints for configuration details.

Sample implementation: SpacePointSolver module

The SpacePointSolver module produces up to three collections of charged space points, and associations between space points and hits.
The transformation from the internal structure SpaceCharge to each of the data products (recob::SpacePoint and now also recob::PointCharge) is performed in a single method, and another method takes care of the associations only.
My changes use the recob::ChargedSpacePointsCollectionCreator tool to write the data products. I have added a method to populate the data products (not the associations) from a space charge. I have changed the method converting the results to the data product to use it, and I rescoped the other method, which used to produce only the associations, to create the charged space points and the associations to hits at the same time. This is more robust since the order of space points is guaranteed to be consistent.
I have also
  • swapped the space point and hit sides of the association; the idea is that the produced associations are in the order of the left element ("key"). This has probably no effect.
  • replaced the use of util::CreateAssns() with the simpler art::PtrMaker (actually used internally by the writing tool) and the art::Assns::addSingle() method. This is the recommended practice and I believe it's much clearer now.

#6 Updated by Christopher Backhouse almost 2 years ago

I've read through all the code and it looks reasonable (including the changes to SpacePointSolver), modulo the inevitable disagreements about style. What are our conventions for this? Do I get to homogenize SpacePointSolver to my style because it's my code, or we leave it as a mish-mash, or there's some style oracle to consult?

I agree that CreateAssns() is more confusing than just doing things directly. I didn't realize there was so little magic to it.

I'll try and set up a test now.

A couple of code comments:

In ChargedSpacePoints.h there are some documentation glitches:

 * Currently there are three different type of proxy-like objects for tracks.
 * Each one supports a specific concept:

and

    /// Returns the position of the space point.
    bool hasCharge() const { return chargeInfo().hasCharge(); }

seem to be copy-paste errors.

In ChargedSpacePointCollectionCreator, why are fSpacePoints and fCharges pointers to vectors rather than simply vectors? The extra indirection doesn't seem to be necessary.

#7 Updated by Christopher Backhouse almost 2 years ago

And confirmed that with these branches the reconstruction looks the same as before and the extra PointCharge product gets written to the file.

#8 Updated by Gianluca Petrillo almost 2 years ago

I fixed the documentation errors. Thank you for taking a look to that.

Christopher Backhouse wrote:

I've read through all the code and it looks reasonable (including the changes to SpacePointSolver), modulo the inevitable disagreements about style. What are our conventions for this? Do I get to homogenize SpacePointSolver to my style because it's my code, or we leave it as a mish-mash, or there's some style oracle to consult?

As long as you maintain it, you own the code. LArSoft has some style recommendations which are just that, and touch only a few aspects.
My personal approach tends to be first ask the author of the changes violating my aesthetics, and then take a decision or restyling back.

In ChargedSpacePointCollectionCreator, why are fSpacePoints and fCharges pointers to vectors rather than simply vectors? The extra indirection doesn't seem to be necessary.

Strictly, it is not necessary if you accept the cost of one move. The end of life of those objects is to be wrapped in a unique_ptr to be given to art::Event::put().
If I used vector directly, I'd end up with something like:

auto fPointsForEvent = std::make_unique<std::vector<recob::SpacePoint>>(std::move(fSpacePoints));

I deemed the overhead of the indirection you describe as acceptable because it happens only once for each point, and the cost of reconstructing a point is way higher that that of the indirection. As a bonus, I get an error-free way to determine whether the data has already been moved into the event (the spent() method), while maintaining a separate flag is potentially error prone.

#9 Updated by Christopher Backhouse almost 2 years ago

OK, I buy the pointer argument. Though now I'm wondering why art requires a unique_ptr and won't just let you move() it a vector directly.

#10 Updated by Gianluca Petrillo almost 2 years ago

  • Status changed from Feedback to Resolved
  • % Done changed from 90 to 100

The changes is merged in the release v06_63_00.

#11 Updated by Gianluca Petrillo almost 2 years ago

  • Status changed from Resolved to Closed


Also available in: Atom PDF