-
Notifications
You must be signed in to change notification settings - Fork 37
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
Rawhits removed from TrackerHits #382
Comments
Hi @wdconinc, thanks for the report. That was definitely an unintended consequence of that change. The main goal was to get rid of the duplication of A few questions to see how we can salvage this best:
|
Yeah, and that main goal is what I got from the PR title, and I agree its duplication that should be removed. I was surprised when it turned out to include other changes that I didn't expect.
We are using an EDM4eic RawTrackerHit since there is no similar type available in EDM4hep. https://github.com/eic/EDM4eic/blob/09d18ef63f8b18ecc94ce995d812ab90d597acc0/edm4eic.yaml#L351. We have an EDM4eic TrackerHit too which we use in some subsystes but not all, and we would prefer to move all to EDM4hep instead of the other way round. So, one type, but outside EDM4hep. Main goal to have this is to have a full chain of required relations from reconstructed to raw detected quantities (and only optional associations/links to truth info).
At this point we store the raw hits and they are accessible and used by analysis scripts based on what is them stored in the root file (primarily things like how many raw hits are unused, or used twice). Since it is clear which collection is referred to (only one raw hits collection for each TrackerHitCollection), only the index is used, just as with how other vector members are accessed in root/uproot.
We would be ok with accessing this through an the interface instead of the type. Also could be an interface itself, i.e. RawTrackerHit being an interface for various ways of storing raw hit data. I think in either case this will result in a new hard requirement on a next version of EDM4hep.
The complication here is that we would need to have the equivalent of our EDM4eic RawTrackerHit as an option for the raw hit data type, since once the data type is defined in EDM4hep, I don't think we can add more types to it in EDM4eic, right? |
We might have gone for the easiest way with removing the I am currently not entirely sure how to best fix this without delaying EDM4hep v1.0 even further. Let me confirm that the schema evolution mechanism of podio can actually handle an addition of a From a usability in frameworks point of view: I am fairly certain that in the k4FWCore implementation that we currently have, we cannot simply ask a/the data service for a collection by its ID rather than its name. In "plain podio" it's possible to do at the moment (by first getting the name from the frame by ID and then getting the collection from there). I am not sure how Jana2 handles this. Obviously if we have an interface for raw hits that would be a lot easier because than the relation handling is done completely under the hood and "will just work (TM)".
I think the only real contender for a similar thing in EDM4hep would be the
Yeah, for interfaces, we need all participating types to be known when the interface is defined, see also this discussion / comment in podio |
We have AIDASoft/podio#714 in flght and it should be merged in time for the next tag of podio (hopefully this week). That would at least allow you to define an interface in EDM4eic that uses types from both EDM4hep and EDM4eic. That doesn't fully solve the issue under discussion here, but it might make the amount of redefinition slightly less. We had a brief discussion in the EDM4hep meeting today and at least for v01-00 we don't really see a way to bring back the Another alternative for similar functionality would be to use templated links for connecting the tracker hits with the raw hits. Since you only have one raw hit type at the moment, that might be a possibility to keep at least some of the indexing approach alive. However, I don't know enough about your particular details to know for sure. |
TL;DR: Would you be persuadable to use We had quite some discussion last week about this issue and various ways of how to solve the general case of "attaching raw (hit) information to higher level datatypes". The main issue we had in mind in that discussion is that most likely the higher level datatype will already be in EDM4hep, while some of the raw information will be very detector specific. Hence, we will most likely not be able to know about all of them and they will usually live in some extension (at least for a considerable amount of time, until they have been refined enough to be upstreamed). The technical solutions to this that we discussed were:
Proposed solution:
|
This was not quite the descriptive PR title that I thought it was, and it is turning out to be quite annoying to deal with for us due to the removal of the vector member of raw hits in TrackerHit. Same for ACTS, which is relying on the raw hits. Edit: ACTS abuses this somewhat, but we are using this in the intended way: to refer to raw hits.
Originally posted by @wdconinc in #261 (comment)
The text was updated successfully, but these errors were encountered: