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

feat: migrate PhotoMultiplierDigi_factory to JOmniFactory #1177

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Dec 6, 2023

Briefly, what does this PR introduce?

This migrates PhotoMultiplierDigi_factory to JOmniFactory. Couple of points of note:

  • src/global/digi now has only headers, so the target digi_library is gone; probably could move PhotoMultiplierDigi_factory into src/factories/digi for clarity,
  • JANA doesn't support parsing of std::vector<std::pair<double, double>> for quantum efficiency, so that's disabled in the ParameterRef list (@nathanwbrei OT, but having to specify a variable name for these ParameterRef seems useless; can we stick them in a container?),
  • the passing of a lambda which captures this and calls m_RichGeoSvc().GetReadoutGeo(GetPluginName()) which in turn has a call_once is janky (but not a change from previous practice...):
    • m_RichGeoSvc() is implicitly modifying the service with call_once (and apparently allowed to; @nathanwbrei can JANA enforce immutability of services?)
    • the RichGeo service as used by the DRICH plugin and the RichGeo service as used by the DIRC plugin are going to be different, maybe, and whoever calls GetReadoutGeo() first gets the call_once flag; that seems dangerous. Putting both PhotoMultiplierDigi_factories in the same plugin doesn't seem like it would be possible. The RichGeo likely needs to be able to support more than one ReadoutGeo and initialize them on demand. It probably needs to fill a map by detector name (not plugin name), with a map of call_once flags or so. @c-dilks any thoughts on where you were going with that?

I think for now this maintains the status-quo, but we need to find a better solution to this in a different PR that reworks this...

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • 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?

No.

Does this PR change default behavior?

No.

@github-actions github-actions bot added topic: tracking Relates to tracking reconstruction topic: PID Relates to PID reconstruction topic: infrastructure topic: barrel labels Dec 6, 2023
@wdconinc
Copy link
Contributor Author

wdconinc commented Dec 6, 2023

No diffs.

@wdconinc wdconinc marked this pull request as ready for review December 6, 2023 23:47
@wdconinc wdconinc mentioned this pull request Dec 6, 2023
23 tasks
@c-dilks
Copy link
Member

c-dilks commented Dec 7, 2023

@c-dilks any thoughts on where you were going with that?

One call_once flag for each detector should do it.

The overall design rule I tried to enforce here is "keep detector-dependence out of the algorithms." richgeo is supposed to serve the detector dependent stuff to the algorithms, but it's not as clean as I wanted it to be, as you can see from some of our factories.

@wdconinc
Copy link
Contributor Author

wdconinc commented Dec 7, 2023

"keep detector-dependence out of the algorithms."

Definitely appreciate this design philosophy! (and the difficulties that come with it...)

@wdconinc wdconinc force-pushed the photo-multiplier-digi-factory-to-omni-factory branch from d8c9608 to 61cb5c4 Compare January 10, 2024 20:51
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

@wdconinc wdconinc added this pull request to the merge queue Jan 10, 2024
Merged via the queue into main with commit 3b8d579 Jan 10, 2024
72 checks passed
@wdconinc wdconinc deleted the photo-multiplier-digi-factory-to-omni-factory branch January 10, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: barrel topic: infrastructure topic: PID Relates to PID reconstruction topic: tracking Relates to tracking reconstruction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants