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.

Sign up for GitHub

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

Jump to bottom

Geometry template and barrel extension #35

Merged
merged 35 commits into from
Feb 7, 2023
Merged

Geometry template and barrel extension #35

merged 35 commits into from
Feb 7, 2023

Conversation

ebrondol
Copy link
Collaborator

@ebrondol ebrondol commented Dec 7, 2022 •
edited
Loading

This PR aims to address the following open issues:

BEGINRELEASENOTES

  • Template CLUEAlgo and CLUEAlgoGPU on TILE_CONST
  • Correct treatment of different part of the ECAL detector: EB, EE+ and EE-
  • Phi search extension in CLUEAlgo
    ENDRELEASENOTES

Still to do to finalize this PR:

  • More clear readme.md
  • Recipe on how to add a new detector
  • Clean up code in several places
  • Template on the TILE and insert constants_type_t to access to the variables in the TILE_CONST
    Macro folder also to be cleaned up and make accessible to other developers -> keep it for a later PR

Developments on GPU:

@vvolkl

@vvolkl
Copy link
Contributor

vvolkl commented Dec 12, 2022

With the added suffix, LayerTilesGPUT becomes a bit hard to read, maybe better LayerTilesGPU_T?

Otherwise LGTM.

@ebrondol
Copy link
Collaborator Author

ebrondol commented Dec 12, 2022 •
edited
Loading

With the added suffix, LayerTilesGPUT becomes a bit hard to read, maybe better LayerTilesGPU_T?

Otherwise LGTM.

Thanks, this was addressed in the last commit.

@ebrondol ebrondol changed the title WIP: Geometry template and barrel extension Geometry template and barrel extension Dec 16, 2022
@ebrondol
Copy link
Collaborator Author

From my side this PR is ready.

The tests (and fixes, if needed) on the GPU part can also be done in another PR when the temporary fix on the initializer list for nvcc is merged.

@ebrondol
Copy link
Collaborator Author

@rovere might also want to send a review of this PR before merging

@ebrondol
Copy link
Collaborator Author

ebrondol commented Dec 21, 2022 •
edited
Loading

Hi @vvolkl @tmadlener ,
from my side this PR is ready to go in.
I have reported the situation on the GPU side in the description of the PR but given that the tests are running fine and the compilation as well, I would open an issue and postpone this to a future PR.

@ebrondol
Copy link
Collaborator Author

ebrondol commented Jan 5, 2023

Hi @vvolkl @tmadlener
Any news on the merging of this PR ?
Happy 2023!

@andresailer andresailer self-requested a review January 10, 2023 08:14
@ebrondol ebrondol requested a review from vvolkl January 16, 2023 10:53
andresailer
andresailer reviewed Jan 18, 2023
View reviewed changes
Copy link
Contributor

@andresailer andresailer left a comment

Choose a reason for hiding this comment

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

Generic Comment

  • move the header files to include/clue and #include "clue/...", to separate things a little bit more in common insallations or views
  • Otherwise also mostly minor comments, nothing blocking, I think.

Thanks!

include/LayerTilesConstants.h Show resolved Hide resolved
include/LayerTiles.h Show resolved Hide resolved
include/LayerTiles.h Outdated Show resolved Hide resolved
include/LayerTiles.h Outdated Show resolved Hide resolved
include/LayerTiles.h Outdated Show resolved Hide resolved
src/CLUEAlgo.cc Show resolved Hide resolved
src/CLUEAlgoGPU.cu Show resolved Hide resolved
src/CLUENtuplizer.cpp Show resolved Hide resolved
src/CLUENtuplizer.cpp Outdated Show resolved Hide resolved
src/ClueGaudiAlgorithmWrapper.cpp Outdated Show resolved Hide resolved
@ebrondol
Copy link
Collaborator Author

Hi @andresailer, with regards to the suggestion here below (and other similar ones):

Generic Comment

  • move the header files to include/clue and #include "clue/...", to separate things a little bit more in common insallations or views

We were in an ongoing discussion with @vvolkl about introducing kalos/clue in the key4hep stack so that we could have taken the entire library instead of copy-paste. I would keep this library independent from DD4hep geometry, for example. This is still under discussion and I hope it will be implemented soon.

@ebrondol ebrondol mentioned this pull request Jan 24, 2023
Open
@ebrondol
Copy link
Collaborator Author

Hi @andresailer , if I am not mistaken, I have resolved all comments.
The one related to the possibility of phi being zero needed some changes in the code. It was also resolved today and tested. Please have a look if I missed anything - thank you.

andresailer
andresailer reviewed Feb 7, 2023
View reviewed changes
docs/clic-recipe.md Outdated Show resolved Hide resolved
ebrondol added 15 commits February 7, 2023 17:21
@ebrondol @andresailer
Template CLUEAlgo and CLUEAlgoGPU on TILE_CONST
4eb1afd 
First attempt to template, still with Segm Fault

Extend template to Cupla, still with Segm Fault

Fix memory allocation, it runs correctly now

Total number of layers defined in LTConstants now

Template also the GPU part
@ebrondol @andresailer
Correct CLICdet geom and include CLD geometries
421a001 
Correct parameters for CLICdet

Include CLD geometry

Add template class for CLD
@ebrondol @andresailer
Add totSize and totEnergy in ntuple
d1bd3ef
@ebrondol @andresailer
Include third tree with hits of clusters
f92d73b
@ebrondol @andresailer
Small changes
150d771
@ebrondol @andresailer
Run CLUE twice, once in barrel and once in both endcap
6a846c6
@ebrondol @andresailer
Fix hits in cluster
800b718
@ebrondol @andresailer
Update gaudi wrapper test file
b7e66ec
@ebrondol @andresailer
Separation on EB, EE+ and EE-
283199c 
Change x input in CLUE for barrel

Separate barrel from endcap

Run correctly EE+ and EE-

Consistency checks on x and y added
@ebrondol @andresailer
Add new input file for tests
6c7c325
@ebrondol @andresailer
Phi search extension in CLUEAlgo (not in GPU)
3bdc5c2 
Include phi search in barrel tile

CLD and generic layer updated
@ebrondol @andresailer
Add MCPart energy to validation
9d3d07b
@ebrondol @andresailer
Update clicRec config file and recipes
0ea3be3 
Update clicRec config file after PR#91 in k4MarlinWrapper

Change name to make it more clear

Update recipes and config files
@ebrondol @andresailer
Phi search extension in CLUEAlgo (for GPU)
68552e0 
Include phi search in barrel tile for GPU code

Small fix
@ebrondol @andresailer
First version of some useful macros
140caeb
ebrondol and others added 20 commits February 7, 2023 17:21
@ebrondol @andresailer
Change readme to be more readable
c353353
@ebrondol @andresailer
Small fixes
28fa196
@ebrondol @andresailer
Small fixes
3f7a5ec
@ebrondol @andresailer
Code more readable
4411721
@ebrondol @andresailer
Add recipe on how to include a new detector
85ce783
@ebrondol @andresailer
Formulas in readme
137fc56
@ebrondol @andresailer
First round of clean up and correctly fill layer in cluster ntuples
b0bffa9
@ebrondol @andresailer
Second round of clean up
a3e5b4a
@ebrondol @andresailer
Template on array of tiles instead of on constants
f977d30
@ebrondol @andresailer
Remove macros folder for now
e4307ef
@ebrondol @andresailer
Modify readme accordingly after the changes
16fb2ab
@ebrondol @andresailer
First set of fixes for GPU template
81ea031
@ebrondol @andresailer
Move it to pass by value
7bbc226
@ebrondol @andresailer
Compile but do not run correctly (IMO)
0071170
@ebrondol @andresailer
Include doc to generate at the ECAL surface
519da9f
@ebrondol @andresailer
Introducing CLDBarrel and improve code readibility
bcfedb8
@ebrondol @andresailer
Doxygen style comments and re-include logo
7f3cbe8
@ebrondol @andresailer
Add singleMCPart bool for analysis
02f15b6
@ebrondol @andresailer
Avoid dividing by phi that could be zero
47c14c3
@andresailer
Update docs/clic-recipe.md
6aacca7
@andresailer andresailer force-pushed the geometryTemplateGPU_rebase branch from c78db02 to 6aacca7 Compare February 7, 2023 16:21
@andresailer andresailer enabled auto-merge (rebase) February 7, 2023 16:21
@andresailer andresailer merged commit 70931cf into key4hep:main Feb 7, 2023
@ebrondol ebrondol deleted the geometryTemplateGPU_rebase branch February 13, 2023 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andresailer andresailer andresailer approved these changes

@vvolkl vvolkl Awaiting requested review from vvolkl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extend to use CLUE in barrel Template CLUE class to include multiple geometries
3 participants
@ebrondol @vvolkl @andresailer