-
Notifications
You must be signed in to change notification settings - Fork 19
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
Fleshed out instructions about DD4hep geometry information #164
Conversation
Thanks for opening this PR and clarifying things a bit. Quick question about the error you observe. Is it possible that the |
@tmadlener yes you are right, that was the only problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I basically just have a few proposals to try and clarify this even further, resp. bring it in line with our usual jargon. I am not entirely sure if all of them are immediately clear to newcomers, but you would be in an ideal position to judge that ;)
Edit: I think the doctest
CI workflows are still broken due to another issue, so you don't have to worry about those.
Current CI issues look like I need to add |
Co-authored-by: Thomas Madlener <[email protected]>
We keep track of that here: key4hep/key4hep-spack#547 (a fix is under way but still needs a PR to be merged in spack) |
Co-authored-by: Thomas Madlener <[email protected]>
Co-authored-by: Thomas Madlener <[email protected]>
Co-authored-by: Thomas Madlener <[email protected]>
Question for @andresailer since we are here already. Do I understand correctly that the CellID service needs to come after the geometry service? Or is the order irrelevant and things will also work if I do svcList.append(cellIDSvc)
svcList.append(geoservice) |
Just append to svcList after the service is defined, and the question becomes moot. |
To clarify this commit never made it to the mainline branch, the PR it belongs to was closed and not merged. PS: I deleted the move-cellid branch now from this repository. |
but that might not stop people from defining them in the wrong(?) order. So if the order is important we should maybe clarify that the geometry service has to come first (or at least before the cell ID svc). |
Given cellIDSvc.GeoSvcName = geoservice.name() It should stop them from defining them in the wrong order. |
Excellent point (and good enough for me). If only people would read before they start asking questions ... ;) |
Co-authored-by: Thomas Madlener <[email protected]>
Currently I get the error
I think it might be because
TrackingCellIDEncodingSvc
was added here on September 13th. Then on September the 28th, this commit seems to move it to a different namespace?