Drift chamber digi

[BrieucF]

BEGINRELEASENOTES

• Add a data type for Drift Chamber digitized hits (before the left right ambiguity is resolved)

ENDRELEASENOTES

Tagging @atolosadelgado

• Update README links (once everything has settled)
• Update the "write all" and "read all" scripts to also include some dummy data for the SensitiveWireHit (i.e. this and this)
• Add this to the edm4hep2json here

[BrieucF]

Thanks Andre

[BrieucF]

I will update the README only when the PR will be ready to be merged

[tmadlener]

How confusing will the slightly different meanings of position be in this case? Resp. do we need to worry about this?

[BrieucF]

Yes, if used blindly together with the position of the other data types in this interface, things will go wrong (a 3D hit position is ill-defined for this new data type). We went for the less intrusive option w.r.t. existing edm4hep but we can remove the position from the interface and name it differently for the SensitiveWireHit, if there is a consensus that this is the way to go. Probably a good thing to discuss tomorrow?

[tmadlener]

How would the interpretation of the positionAlongWireError work in this case? IIUC I would need the directionSW to get the direction and then the position to get a point on the wire to get a reference point(?).

[BrieucF]

Correct, the reference point on the wire is position, then to know "in which direction the error is defined" without accessing the geometry, you need sensitiveWireDirection (which I just renamed for more clarity)

[BrieucF]

I have added a proposal for the errors. I also applied @gaede suggestion and made the direction vector 2D (which I also renamed to be more clear). Comments welcome

[BrieucF]

Actually, this data type would be more robust with explicit fields for the wire stereo and azimuthal angle (current proposal updated to this). With the vector2f, users might have different conventions.

[gaede]

As discussed this morning, you could also make this class look very similar to the TrackerHitPlane, e.g. by replacing

- edm4hep::Vector2d sensitiveWireDirection // direction of the sensitive wire (normal vector to the circle)

with

  - edm4hep::Vector2f u            // measurement direction along the wire - (theta, phi) in global coordinates 
  - edm4hep::Vector2f v            // measurement direction orthogonal to the wire -(theta, phi) in global coordinates 
  - float du                       // measurement error along the wire 
  - float dv                       // measurement error of distance to the wire 

this way you'd have the uncertainties in the class, and we would express similar concepts with similar/same names.
Once tracking is done, you could updated v such that m = pos + v is the actual position of the hit base on the track fit...

[tmadlener]

Are the u and v also labelled as global coordinates in the TrackerHitPlane? I though they are to be interpreted as local coordinates?

[gaede]

The directions u and v are meant to be in global coordinates - see: https://ilcsoft.desy.de/LCIO/current/doc/doxygen_api/html/classEVENT_1_1TrackerHitPlane.html. There seems sth. has been lost in translation when copying this over from LCIO. Should update our yaml file...

[BrieucF]

As discussed this morning, you could also make this class look very similar to the TrackerHitPlane, e.g. by replacing

- edm4hep::Vector2d sensitiveWireDirection // direction of the sensitive wire (normal vector to the circle)

with

  - edm4hep::Vector2f u            // measurement direction along the wire - (theta, phi) in global coordinates 
  - edm4hep::Vector2f v            // measurement direction orthogonal to the wire -(theta, phi) in global coordinates 
  - float du                       // measurement error along the wire 
  - float dv                       // measurement error of distance to the wire 

this way you'd have the uncertainties in the class, and we would express similar concepts with similar/same names. Once tracking is done, you could updated v such that m = pos + v is the actual position of the hit base on the track fit...

I don't know what to do here... I am of course in favor of maximizing similarities between data types so that people can use similar concepts in similar ways but I am not sure this is fully relevant here.

Unlike for the TrackerHitPlane, one of the two "error directions" (the one relevant for distanceToWire) is actually not really a direction, it can rather be seen as a plane (one would need to take an arbitrary decision to define a direction). It also brings additional superfluous members since this information can be retrieved from the wire direction itself without accessing the detector geometry (it is the plane perpendicular to it).

Another point is that those directions in sensitive wire detectors have well established names (e.g. stereo angle) and I like to have it explicit in the associated data members, instead of u and v which are not common in this field.

Finally, I fail to see (future may prove me wrong) use cases where one would want to treat "similarly" TrackerHitPlane and SensitiveWireHit as they are fundamentally different: one being a point-like measurement while the other is defining a circle in a 3 dimensional space. The way I see it now is that only what comes after the left/right ambiguity is alleviated would be treated in a similar way as silicon tracker hits.

I think I'd rather keep SensitiveWireHit as it is proposed at the moment.

Maybe something is wrong with my reasoning or I am missing a point. What do you think?

[jmcarcell]

For the inclusion, if this makes it easier for people to use and test it we could add it to EDM4hep and then we can add warnings to make it clear that it is an experimental type (breaking changes will happen), and also not include it in our "official" CI files. Then, no one should be upset if the files that have collections with these are not readable after it changes in EDM4hep. We should foresee this happening after EDM4hep 1.0 and always denying adding a new type until it is completed is quite restrictive. In addition, if most (all?) of the users of the new type are here and are aware it may change then it should be easier. But I can also see a type being added as an experiment forever...

[tmadlener]

Once tracking is done, you could updated v such that m = pos + v is the actual position of the hit base on the track fit...

No such thing as updating after ... in EDM4hep. That would mean cloning + setting a new value in that clone.

I think if things are conceptually different enough then it makes sense to also keep their abstractions different. Choosing the wrong abstraction might do more damage than not abstracting enough. Since this type should represent the digitized measurement that comes out of a drift chamber, maybe we have to make sure it does that first and only as a (somewhat) secondary concern think about how it relates to tracking, resp. how it would be used in the final fitted track as a tracker hit.

With that in mind and also regarding some parts of the discussion in #382, I think we will probably have to think a bit about what a TrackerHit actually is, and also what a "raw tracker hit" could be and how we can connect the two things in the end.

not include it in our "official" CI files. Then, no one should be upset if the files that have collections with these are not readable after it changes in EDM4hep.

I think we should still write them, but potentially not check them on the reading side. The reason I say we should still write them is that we don't want to break the rest of the files simply because they contain collections of datatypes that are not yet stable. Currently podio always reads full frames with no way of just skipping certain collections (until we merge AIDASoft/podio#504). In principle the buffer creation mechanism takes into account the schema version so that at least connecting the buffers to the branches should work correctly even if things would break during actual schema evolution later (in case of unsupported evolutions).

Here we have to sit down and define some sort of policy on how we want to deal with this.

[BrieucF]

Once tracking is done, you could updated v such that m = pos + v is the actual position of the hit base on the track fit...

No such thing as updating after ... in EDM4hep. That would mean cloning + setting a new value in that clone.

I think if things are conceptually different enough then it makes sense to also keep their abstractions different. Choosing the wrong abstraction might do more damage than not abstracting enough. Since this type should represent the digitized measurement that comes out of a drift chamber, maybe we have to make sure it does that first and only as a (somewhat) secondary concern think about how it relates to tracking, resp. how it would be used in the final fitted track as a tracker hit.

With that in mind and also regarding some parts of the discussion in #382, I think we will probably have to think a bit about what a TrackerHit actually is, and also what a "raw tracker hit" could be and how we can connect the two things in the end.

not include it in our "official" CI files. Then, no one should be upset if the files that have collections with these are not readable after it changes in EDM4hep.

I think we should still write them, but potentially not check them on the reading side. The reason I say we should still write them is that we don't want to break the rest of the files simply because they contain collections of datatypes that are not yet stable. Currently podio always reads full frames with no way of just skipping certain collections (until we merge AIDASoft/podio#504). In principle the buffer creation mechanism takes into account the schema version so that at least connecting the buffers to the branches should work correctly even if things would break during actual schema evolution later (in case of unsupported evolutions).

Here we have to sit down and define some sort of policy on how we want to deal with this.

This is interesting and we should indeed have this discussion but it goes somewhat beyond the scope of this pull request. I am afraid we won't converge in the timescale within which this PR is needed. Shall I add 'EXPERIMENTAL' to the Description field for now?