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: use algorithms properties in CalorimeterClusterRecoCoG #1154

Closed
wants to merge 13 commits into from

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Nov 27, 2023

Briefly, what does this PR introduce?

This PR converts CalorimeterClusterRecoCoG to using properties defined as algorithms::Property.

TODO:

Cherry-picked d498abe from #1161.

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.

@wdconinc wdconinc marked this pull request as draft November 27, 2023 02:49
@github-actions github-actions bot added topic: calorimetry relates to calorimetry topic: infrastructure topic: barrel topic: far-forward Far forward reconstruction topic: far-backward Reconstruction related to far backward detectors topic: forward topic: backward and removed topic: infrastructure labels Nov 27, 2023
@wdconinc wdconinc mentioned this pull request Nov 28, 2023
7 tasks
@wdconinc wdconinc marked this pull request as ready for review November 28, 2023 07:30
@wdconinc
Copy link
Contributor Author

@wdconinc
Copy link
Contributor Author

This is ready for review and merging.

veprbl
veprbl previously approved these changes Nov 29, 2023
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.

This worked out nice. Having a map of properties helped a bunch.

@veprbl
Copy link
Member

veprbl commented Nov 29, 2023

The full error message is 4 gigabytes, but can be summarized as:

In file included from /home/runner/work/EICrecon/EICrecon/src/detectors/EEMC/EEMC.cc:13:
/home/runner/work/EICrecon/EICrecon/src/factories/calorimetry/CalorimeterClusterRecoCoG_factoryT.h:66:67: error: 'key' in capture list does not name a variable
            [app, &m_algo = m_algo, &m_log = m_log, param_prefix, key](auto&& val) {
                                                                  ^

@wdconinc
Copy link
Contributor Author

There are new LSAN errors in CI.

Actually, this is more curious (and tl;dr no memory leaks). What happened is that within a pipeline one job checked the geometry version in eic-shell and then decided it didn't need to regenerate the simulation file (cache hit). Then the reconstruction job started in the next job, in its own eic-shell, and it had a newer geometry version due to the cvmfs sync, so it continued to run with the older simulation geometry input file but against newer geometry in memory.

The geometry difference was https://github.com/eic/epic/pull/603/files -> i.e. r,phi segmentation to x,y segmentation. That probably caused many warnings about missing cellIDs (including MaxCellIDerrors failures), and memory errors are probably associated with that. Probably ok to let this slide...

Last checks indicate no memory issues anymore: https://github.com/eic/EICrecon/actions/runs/7038493136/job/19156610394?pr=1154#step:8:2091

@wdconinc
Copy link
Contributor Author

And no diffs, e.g. https://github.com/eic/EICrecon/actions/runs/7050567792/job/19192283374?pr=1154#step:11:106. After 9ee8a85 we should get a clean clang-tidy-iwyu check too. And the eicmkplugin fix is in the merge queue.

So, good for review and merge, as far as I'm concerned.

@wdconinc wdconinc marked this pull request as draft December 1, 2023 04:17
auto-merge was automatically disabled December 1, 2023 04:17

Pull request was converted to draft

@wdconinc
Copy link
Contributor Author

wdconinc commented Dec 1, 2023

This PR has become a bit more difficult now that #865
has been merged. Instead of looping over the algorithm's properties and parsing using Jana itself, we probably need to loop over the algorithm's properties inside JOmniFactory's constructor if constexpr the algorithm has such properties, and then store ParameterRefs in a container of ParameterBase smart pointers. The ParameterRefs will register themselves and get parsed and we don't need to access the container anymore.

@wdconinc
Copy link
Contributor Author

Deferred to #1165

@wdconinc wdconinc closed this Apr 12, 2024
@wdconinc wdconinc deleted the algorithms-properties branch April 12, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: backward topic: barrel topic: calorimetry relates to calorimetry topic: far-backward Reconstruction related to far backward detectors topic: far-forward Far forward reconstruction topic: forward topic: infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants