extend for drift chamber and TPC #179

wenxingfang · 2022-10-17T08:51:18Z

BEGINRELEASENOTES

Extend for drift chamber and TPC study
The extended EDMs are tested within CEPCSW, and it works well
The extended edm4hep::TrackerPulse and edm4hep::TrackerData are similar to what they are in ILC TPC

ENDRELEASENOTES

tmadlener

See also discussion in EDM4hep meeting

I have one more technical comment, that I did not raise during the meeting. Could you make all integer types a fixed width integer, especially the unsigned long long are somewhat loosely defined with only a minimal size and might change width depending on compiler, platform, etc. We have done this exercise for all other datatypes in EDM4hep, so for consistency it would be nice if it can also be done here.

edm4hep.yaml

andresailer

Similar to Thomas' comment about fixed length integer, and similar things. A few proposal for renaming to be consistent with other types with the same fields.

Except we have both "EDep" and "eDep" in existing types already...

edm4hep.yaml

andresailer · 2022-10-18T07:15:05Z

edm4hep.yaml

+      - edm4hep::Vector3d position           //the primary ionization's position in [mm].
+      - int   type                           //type.
+    VectorMembers:
+       - unsigned long long electronCellID   //cell id.


Suggested change

- unsigned long long electronCellID //cell id.

- uint64_t electronCellID //cell id.

Which cellID is this?

as ionized electrons from the same primary ionization cluster may be produced in different drift chamber cells, especially when they are produced close to the edge of a cell. So each ionized electron should have one cell id.

edm4hep.yaml

wenxingfang

to solve issue #25

wenxingfang · 2022-11-01T09:20:14Z

Hi @gaede , I renamed 'RecDndx' to 'RecDqdx' and used edm4hep::Quantity dQdx to save the type, value, and error. What is your opinion?

wenxingfang · 2022-12-13T09:06:38Z

@andresailer Hi Andre, now the checks are successful. Do you think if this pull request can be merged?

tmadlener · 2022-12-14T10:46:13Z

It looks like there are still a few formatting and whitespace issues. Could you fix them as well?

https://github.com/key4hep/EDM4hep/actions/runs/3683601506/jobs/6232363223#step:4:526 (order of headers)
https://github.com/key4hep/EDM4hep/actions/runs/3683601506/jobs/6232363223#step:4:537 (order of headers)
https://github.com/key4hep/EDM4hep/actions/runs/3683601506/jobs/6232363223#step:4:556 (looks like mainly trailing whitespaces here)

wenxingfang · 2022-12-14T13:09:22Z

@tmadlener @andresailer, This time all the checks are successful. It seems at least 1 approving review is needed in order to merge the PR.

andresailer · 2023-01-31T08:07:03Z

edm4hep.yaml

- #----------  TPCHit
-  edm4hep::TPCHit:
+ #---------- TrackerRawData
+  edm4hep::TrackerRawData:
    Description: "Time Projection Chamber Hit"


This description should be updated

ok, how about using "Raw data of a tracker hit"?

andresailer · 2023-01-31T08:07:15Z

edm4hep.yaml

+      Members:
+        - uint64_t cellID  // cell id
+        - uint32_t N       // number of reconstructed ionization cluster.
+        - float  eDep        // energy deposit.


Suggested change

- float eDep // energy deposit.

- float eDep // energy deposit [GeV].

eDep is the reconstructed energy deposit?

yes, it should be a reconstructed energy deposit.

andresailer · 2023-01-31T08:07:50Z

edm4hep.yaml

+    Author : "Wenxing Fang, IHEP"
+    Members:
+       - uint64_t cellID                 //cell id.
+       - float time                      //time.


Suggested change

- float time //time.

- float time //time [ns].

andresailer · 2023-01-31T08:17:14Z

edm4hep.yaml

+        - float expected   // expected value
+        - float sigma      // sigma value
+
+    # hit level data


Suggested change

# hit level data

# Reconstructed hit information

andresailer · 2023-01-31T08:18:33Z

edm4hep.yaml

+       - int16_t quality                 //quality.
+       - std::array<float,3> covMatrix   //lower triangle covariance matrix of the charge(c) and time(t) measurements.
+    OneToOneRelations:
+      - edm4hep::TrackerData trackerData  //Optionaly, the TrackerData that has been used to create the pulse can be stored with the pulse.


Suggested change

- edm4hep::TrackerData trackerData //Optionaly, the TrackerData that has been used to create the pulse can be stored with the pulse.

- edm4hep::TrackerData trackerData //Optionally, the TrackerData that has been used to create the pulse can be stored with the pulse.

andresailer · 2023-01-31T08:21:24Z

edm4hep.yaml

+    Author : "Wenxing Fang, IHEP"
+    Members:
+       - uint64_t cellID            //cell id.
+       - float time                 //begin time.


Suggested change

- float time //begin time.

- float time //begin time [ns].

andresailer · 2023-01-31T08:21:42Z

edm4hep.yaml

+
+  #-------------  RecDqdx
+  edm4hep::RecDqdx:
+    Description : "dN/dx or dEdx info of Track."


Suggested change

Description : "dN/dx or dEdx info of Track."

Description : "dN/dx or dE/dx info of Track."

andresailer · 2023-02-06T09:56:26Z

edm4hep.yaml

+    Members:
+       - uint64_t cellID                 //cell id.
+       - float time                      //time.
+       - float charge                    //charge.


The charge is in coulomb or multiple or electron (or proton) charges?

It is in coulomb.

andresailer · 2023-02-06T09:56:46Z

edm4hep.yaml

+       - float time                 //begin time.
+       - float interval             //interval of each sampling in [ns].
+    VectorMembers:
+       - float chargeValue          //charge value.


will add a unit with [fC]

wenxingfang · 2023-02-07T03:37:57Z

@andresailer Thanks for your suggestions! The PR has been updated. Please let me know if you have any questions or suggestions.

andresailer · 2023-02-07T08:53:27Z

edm4hep.yaml

+       - edm4hep::TrackerPulse trackerPulse //the TrackerPulse used to create the ionization cluster.
+
+ #----------  TrackerData
+  edm4hep::TrackerData:


Suggested change

edm4hep::TrackerData:

edm4hep::TimeSeries:

andresailer · 2023-02-07T08:53:58Z

edm4hep.yaml

-  edm4hep::TPCHit:
-    Description: "Time Projection Chamber Hit"
+ #---------- TrackerRawData
+  edm4hep::TrackerRawData:


Suggested change

edm4hep::TrackerRawData:

edm4hep::ADCTimeSeries:

- - int32_t rawDataWords //raw data (32-bit) word at i. + - int32_t adcCounts //raw data (32-bit) word at i.

Need an entry for interval?

I am wondering whether ADCTimeSeries is not making this too specific? (In this case it is one, but in other cases a more general concept might be better, instead of having to introduce another type later?)

I am wondering whether ADCTimeSeries is not making this too specific? (In this case it is one, but in other cases a more general concept might be better, instead of having to introduce another type later?)

How about using "RawTimeSeries"?

Need an entry for interval?

I think it is needed.

andresailer · 2023-02-07T08:54:58Z

edm4hep.yaml

-    Description: "Time Projection Chamber Hit"
+ #---------- TrackerRawData
+  edm4hep::TrackerRawData:
+    Description: "Raw data of a tracker hit"


Suggested change

Description: "Raw data of a tracker hit"

Description: "Raw data of a detector read out"

andresailer · 2023-02-07T08:55:50Z

edm4hep.yaml

+       - float time                 //begin time [ns].
+       - float interval             //interval of each sampling [ns].
+    VectorMembers:
+       - float chargeValue          //charge value [fC].


Suggested change

- float chargeValue //charge value [fC].

- float amplitude // calibrated detector data.

andresailer · 2023-02-07T08:56:49Z

edm4hep.yaml

+
+ #----------  TrackerData
+  edm4hep::TrackerData:
+    Description: "TrackerData"


Suggested change

Description: "TrackerData"

Description: "Calibrated Detector Data"

tmadlener · 2023-02-07T16:56:04Z

edm4hep.yaml

+      Members:
+        - uint64_t cellID  // cell id
+        - uint32_t N       // number of reconstructed ionization cluster.
+        - float eDep        // energy deposited [GeV].


Suggested change

- float eDep // energy deposited [GeV].

- float eDep // reconstructed energy deposit [GeV].

to further clarify according to: #179 (comment)

tmadlener · 2023-02-07T17:04:34Z

edm4hep.yaml

-  edm4hep::TPCHit:
-    Description: "Time Projection Chamber Hit"
+ #---------- TrackerRawData
+  edm4hep::TrackerRawData:


I am wondering whether ADCTimeSeries is not making this too specific? (In this case it is one, but in other cases a more general concept might be better, instead of having to introduce another type later?)

wenxingfang · 2023-02-08T02:51:42Z

@andresailer @tmadlener, thanks for the comments! The PR has been updated. Welcome with further suggestions!

jmcarcell · 2023-02-08T07:22:24Z

This is a possibly incomplete list of repos with files that will have to be changed: k4EDM4hep2LcioConv, k4MarlinWrapper, k4LCIOReader, key4DCMTSim and there is an example in k4geo. There is also README.md (there is a link to TPCHit) in this repo that will have to be changed, could you add that last change to this PR @wenxingfang for completeness?
For the other changes I would start by renaming TPCHitCollection and then TPCHit in this order and rawDataWords and I think that should (almost?) work

tmadlener

Thanks for all the updates and the patience. This looks good to me now.

It would be nice if we can address the necessary README changes still in this PR, but since we have to update the diagram as well, I would also be OK with doing that in a separate PR, unless @andresailer has a strong opinion on that.

For the repositories that break with these changes mentioned by @jmcarcell, I suppose most of the changes should be rather straight forward fixes, and we could prepare those PRs before we merge this and then do a coherent build for all of them to see if we didn't miss anything.

jmcarcell · 2023-03-01T21:19:35Z

After running a consistent build and finding no errors in https://github.com/key4hep/EDM4hep/actions/runs/4306734737/jobs/7510924036 (see downstream-build, the current error happens due to some issue when fetching for a different package that is not affected by this change but was included by the build) I'm decently confident we have found all the places inside key4hep that are affected. It's time to press those green buttons, and maybe the nightly will pick it up today

wenxingfang · 2023-03-02T00:38:45Z

@jmcarcell Thanks a lot! Who should press the green button?

jmcarcell · 2023-03-02T07:28:08Z

It's done now @wenxingfang

tmadlener reviewed Oct 18, 2022

View reviewed changes

edm4hep.yaml Outdated Show resolved Hide resolved

tmadlener mentioned this pull request Oct 18, 2022

TPCHit too specific? #25

Closed

andresailer reviewed Oct 18, 2022

View reviewed changes

tmadlener reviewed Oct 18, 2022

View reviewed changes

edm4hep.yaml Outdated Show resolved Hide resolved

edm4hep.yaml Show resolved Hide resolved

edm4hep.yaml Outdated Show resolved Hide resolved

tmadlener mentioned this pull request Oct 18, 2022

Inconsistent naming of energy deposit in different types #180

Closed

wenxingfang commented Oct 18, 2022

View reviewed changes

tmadlener linked an issue Oct 18, 2022 that may be closed by this pull request

TPCHit too specific? #25

Closed

andresailer force-pushed the master branch from f59e5ee to 334649c Compare December 12, 2022 13:08

andresailer reviewed Feb 6, 2023

View reviewed changes

andresailer approved these changes Feb 7, 2023

View reviewed changes

andresailer requested a review from tmadlener February 7, 2023 08:37

andresailer reviewed Feb 7, 2023

View reviewed changes

tmadlener reviewed Feb 7, 2023

View reviewed changes

tmadlener reviewed Feb 8, 2023

View reviewed changes

tmadlener added the breaking change Needs changes in dependencies label Feb 8, 2023

This was referenced Feb 14, 2023

Rename TPCHit -> RawTimeSeries key4hep/k4MarlinWrapper#108

Merged

Rename TPCHit -> RawTimeSeries key4hep/k4EDM4hep2LcioConv#8

Merged

Rename TPCHit->RawTimeSeries key4hep/k4LCIOReader#29

Merged

Rename TPCHit -> RawTimeSeries key4hep/key4DCMTSim#2

Closed

jmcarcell mentioned this pull request Feb 28, 2023

Renaming of TPCHit in EDM4hep eic/EICrecon#515

Closed

wenxingfang added 16 commits March 2, 2023 08:10

extend for drift chamber and TPC

840569f

updated accroding the suggestions from edm4hep meeting

579fcf5

using std::array for hypothesis

8c25db5

rename TPCHit to TrackerRawData

80f03aa

adjust RecDndx to be used both for dNdx and dEdx

7377945

test directory updated with TrackerRawData

6010dfa

fix formatting and whitespace issues

730835e

fix order of headers issues

cdfa169

updates on units and descriptions

ac3e2cb

update with class names

3e5a5fd

update TrackerRawData to RawTimeSeries

45feec6

update with clang-format

718d83d

update README.md

1dccb62

update README.md with extended EDMs

f3c41d0

solve conflict

979c485

update TPCHitCollection to RawTimeSeries

e9925d9

jmcarcell force-pushed the master branch from 427746b to e9925d9 Compare March 2, 2023 07:16

jmcarcell merged commit 59b7c23 into key4hep:master Mar 2, 2023

SanghyunKo mentioned this pull request Apr 25, 2023

Migrate from user-defined SparseVector to EDM4hep's TimeSeries HEP-FCC/dual-readout#33

Merged

jmcarcell linked an issue Apr 25, 2023 that may be closed by this pull request

Extend edm4hep::TrackerHit and edm4hep::Track for drift chamber study #124

Closed

jmcarcell mentioned this pull request Apr 25, 2023

Extend edm4hep::TrackerHit and edm4hep::Track for drift chamber study #124

Closed

tmadlener mentioned this pull request Jun 7, 2023

Change capitalization of subdetectorHitNumbers vector members key4hep/k4EDM4hep2LcioConv#16

Merged

BrieucF mentioned this pull request Jul 6, 2023

Add a OneToMany relation from TrackerHit to RecIonizationCluster #211

Closed

This was referenced Jun 18, 2024

RawTimeSeries is not connected to the rest of EDM4hep #316

Closed

Better integration of datatypes for the drift chamber study of CEPC into EDM4hep #323

Open

tmadlener mentioned this pull request Jun 27, 2024

Remove some of the drift chamber study types again #333

Merged

1 task

	- unsigned long long electronCellID //cell id.
	- uint64_t electronCellID //cell id.

	- float eDep // energy deposit.
	- float eDep // energy deposit [GeV].

	- edm4hep::TrackerData trackerData //Optionaly, the TrackerData that has been used to create the pulse can be stored with the pulse.
	- edm4hep::TrackerData trackerData //Optionally, the TrackerData that has been used to create the pulse can be stored with the pulse.

	Description : "dN/dx or dEdx info of Track."
	Description : "dN/dx or dE/dx info of Track."

	Description: "Raw data of a tracker hit"
	Description: "Raw data of a detector read out"

	- float chargeValue //charge value [fC].
	- float amplitude // calibrated detector data.

	Description: "TrackerData"
	Description: "Calibrated Detector Data"

	- float eDep // energy deposited [GeV].
	- float eDep // reconstructed energy deposit [GeV].

extend for drift chamber and TPC #179

extend for drift chamber and TPC #179

Conversation

wenxingfang commented Oct 17, 2022

tmadlener left a comment

Choose a reason for hiding this comment

andresailer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenxingfang left a comment

Choose a reason for hiding this comment

wenxingfang commented Nov 1, 2022

wenxingfang commented Dec 13, 2022

tmadlener commented Dec 14, 2022

wenxingfang commented Dec 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenxingfang commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenxingfang commented Feb 8, 2023

jmcarcell commented Feb 8, 2023 • edited Loading

tmadlener left a comment

Choose a reason for hiding this comment

jmcarcell commented Mar 1, 2023 • edited Loading

wenxingfang commented Mar 2, 2023

jmcarcell commented Mar 2, 2023

jmcarcell commented Feb 8, 2023 •

edited

Loading

jmcarcell commented Mar 1, 2023 •

edited

Loading