Skip to content

[WIP] Adapt the IActsGeoSvc interface to fit better to Acts downstream needs - #35

Closed
tmadlener wants to merge 3 commits into
key4hep:mainfrom
tmadlener:actsgeosvc-interface
Closed

tmadlener wants to merge 3 commits into
key4hep:mainfrom
tmadlener:actsgeosvc-interface

Conversation

@tmadlener

Copy link
Copy Markdown
Member

BEGINRELEASENOTES

  • Make the IActsGeoSvc return the tracking geometry as a shared_ptr to better align with downstream usage
  • Add a magneticField method to the IActsGeoSvc and return a constant B field for now.

ENDRELEASENOTES

@wdconinc Do I remember correctly that you are still using this and this will probably be breaking changes?

Most of the Acts downstream assumes a shared_ptr, so we might as well
return one from here
Make the ActsGeoSvc implementation return a constant magnetic field
along for now.
@wdconinc

wdconinc commented Dec 3, 2025

Copy link
Copy Markdown
Contributor

Do I remember correctly that you are still using this and this will probably be breaking changes?

We're only using this in legacy mode. We have the permanent desire to get back to improving our gaudi support since all our algorithms are written in a framework-agnostic and this is exercised through a gaudi pipeline, but it's obviously a lower priority.

As long as there's a release before major changes take effect, we can pin on that. It may be there already... Or we can pin on a commit too.

@tmadlener

Copy link
Copy Markdown
Member Author

Sounds good. We have https://github.com/key4hep/k4ActsTracking/releases/tag/v00-01 from end of October. No behavior changing changes have landed since, so IIUC we could break things for the next tag, right?

@wdconinc

wdconinc commented Dec 3, 2025

Copy link
Copy Markdown
Contributor

so IIUC we could break things for the next tag, right?

Break all you want :-)


class GAUDI_API IActsGeoSvc : virtual public IService {
public:
using VolumeSurfaceMap = std::unordered_map<uint64_t, const Acts::Surface*>;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
using CellIDSurfaceMap = std::unordered_map<uint64_t, const Acts::Surface*>;

@tmadlener

Copy link
Copy Markdown
Member Author

Closing this in favor of #36, where all of these changes have a better motivation to be given that the original implementation of the ActsGeoSvc was not really used by the functionality added in #25

@tmadlener tmadlener closed this Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants