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

fix: use unique_ptr for geometry in DD4hep_service, hand out not_null const pointers #1045

Merged
merged 9 commits into from
Oct 5, 2023

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Oct 4, 2023

Briefly, what does this PR introduce?

While looking into #1032, #1035, #1036 I started realizing two things:

  • cell ID position converters are created by algorithms from geometry instead of doing this once in the service,
  • there is no const protection in the detector pointers passed to algorithms (i.e. an algorithm could change geometry),
  • there is no protection against null pointers being passed from geometry.

The first two points are related since the interface for creating a cell ID position converter requires a non-const detector (see also AIDASoft/DD4hep#1148).

This PR centralizes the creation of cell ID position converters, clarifies ownership through unique_ptrs, and makes sure no null pointers are passed to the algorithms. In places where the constructor gets the const not_null pointer, we can also impose this in the class on the relevant member variable.

What kind of change does this PR introduce?

  • Bug fix (issue: avoid creation of cell ID position converters in algorithms)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators:

Does this PR introduce breaking changes? What changes might users need to make to their code?

Yes, now requires GSL (which has always been in eic-shell and is a requirement).

Does this PR change default behavior?

No.

@github-actions github-actions bot added topic: calorimetry relates to calorimetry topic: tracking Relates to tracking reconstruction topic: infrastructure labels Oct 4, 2023
@wdconinc wdconinc force-pushed the shared-ptr-const-detector branch from 5468569 to ad97536 Compare October 4, 2023 15:44
@wdconinc wdconinc force-pushed the shared-ptr-const-detector branch from ad97536 to 83d2993 Compare October 4, 2023 15:45
@wdconinc wdconinc temporarily deployed to github-pages October 4, 2023 17:28 — with GitHub Actions Inactive
@veprbl
Copy link
Member

veprbl commented Oct 4, 2023

Do we really want to smear non-null check across our interfaces? Is it not enough to just ensure that the geometry service doesn't return null.

@wdconinc
Copy link
Contributor Author

wdconinc commented Oct 4, 2023

Do we really want to smear non-null check across our interfaces? Is it not enough to just ensure that the geometry service doesn't return null.

It's heavy on notation, I admit, but the requirement for a not_null pointer is in this case imposed by the algorithms, not by the service. The DD4hep_service can satisfy the demand by promising to return a not_null pointer. Putting the requirement in the algorithms makes clear to anyone working in those algorithms that detector can be relied upon to be not_null (yeah, this benefit it kinda negated by the whole init-function and clearer in the RichGeo_service).

@c-dilks c-dilks temporarily deployed to github-pages October 5, 2023 17:33 — with GitHub Actions Inactive
@wdconinc
Copy link
Contributor Author

wdconinc commented Oct 5, 2023

@veprbl I removed the gsl::not_null on all algorithms init calls in b9b97f4

This doesn't really add much utility on the init calls if we can't set it on the class members. I've kept it in DD4hepBfield since there we construct with the detector geometry provided as constructor argument (similar to RichGeo_services).

@c-dilks
Copy link
Member

c-dilks commented Oct 5, 2023

The dRICH benchmarks look okay with this PR.

@wdconinc wdconinc temporarily deployed to github-pages October 5, 2023 20:16 — with GitHub Actions Inactive
@veprbl
Copy link
Member

veprbl commented Oct 5, 2023

@veprbl I removed the gsl::not_null on all algorithms init calls in b9b97f4

This doesn't really add much utility on the init calls if we can't set it on the class members. I've kept it in DD4hepBfield since there we construct with the detector geometry provided as constructor argument (similar to RichGeo_services).

Yes. That was my point, that it's probably the return values where gsl::not_null makes sense (I don't claim any deep philosophical understanding of GSL, though).

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
@wdconinc wdconinc requested a review from veprbl October 5, 2023 21:23
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

Diff LGTM, capybara happy

@wdconinc wdconinc added this pull request to the merge queue Oct 5, 2023
@wdconinc wdconinc temporarily deployed to github-pages October 5, 2023 22:28 — with GitHub Actions Inactive
Merged via the queue into main with commit 0596722 Oct 5, 2023
94 checks passed
@wdconinc wdconinc deleted the shared-ptr-const-detector branch October 5, 2023 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: calorimetry relates to calorimetry topic: infrastructure topic: tracking Relates to tracking reconstruction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants