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

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

Open
tmadlener opened this issue Jun 18, 2024 · 1 comment
Labels
discussion Discussion item

Comments

@tmadlener
Copy link
Contributor

There are a few inconsistencies in the datatypes that were introduced in #179 for the drift chamber study. A tangential but related discussion is also in #322. Opening this issue as a sort of summary discussion board to see if this situation can be improved, also keeping in mind the proposed new datatypes in #299

My main complaints for these are:

  • HitLevelData is a rather generic lwo level datatype to store pretty much any detector hit / readout. However, it has a few members that make it very gaseous detector specific. I think this should be reflected in the name

    EDM4hep/edm4hep.yaml

    Lines 239 to 244 in bd9f450

    edm4hep::HitLevelData:
    Members:
    - uint64_t cellID // cell id
    - uint32_t N // number of reconstructed ionization cluster
    - float eDep [GeV] // reconstructed energy deposit
    - float pathLength [mm] // track path length

  • Hypothesis is not properly documented and seems to be lacking a ndf field(?). Specifically it has a chi2 value, but it's not really documented what this chi2 value represents. Also the sigma docstring could be refined.

    EDM4hep/edm4hep.yaml

    Lines 232 to 236 in bd9f450

    edm4hep::Hypothesis:
    Members:
    - float chi2 // chi2
    - float expected // expected value
    - float sigma // sigma value

  • Overall the structure of the dataypes is fairly involved, but also quite separate from the rest of EDM4hep and I am wondering whether there is a way to (re-)organize the information in a cleaner way in order to make them better integrated into the rest of EDM4hep. Relation diagram of the datatypes and some EDM4hep datatypes:
    image

@tmadlener tmadlener added enhancement New feature or request discussion Discussion item and removed enhancement New feature or request labels Jun 18, 2024
@tmadlener
Copy link
Contributor Author

This is a brief summary the outcome of todays meeting, where we also discussed this (@mirguest, please correct me if anything is wrong in the following).

CEPCSW is currently using a "frozen" version of EDM4hep and other externals since they are heavily involved in preparing the TDR. As such, at least from that point of view we (as in EDM4hep) are pretty much at liberty to break things as far as CEPCSW is concerned. We have then decided to remove some of the drift chamber study related datatypes for EDM4hep v1.0 and bring them back in a more consistent form afterwards. For that we also need to consult with IDEA / FCC reconstruction developers (@BrieucF just as a heads up) and check how to bring everything into a consistent shape, also considering the discussion / resolution in #322. CEPCSW should then be able to "catch up" / migrate from the TDR status to EDM4hep v1.0, once the TDR is done.

Keeping this open, but moving it out of the EDM4hep v1.0 project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion item
Projects
None yet
Development

No branches or pull requests

1 participant