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

expanded version of CreateCaloCells that also add proper cell positions #100

Merged

Conversation

giovannimarchiori
Copy link
Contributor

@giovannimarchiori giovannimarchiori commented Jul 24, 2024

This PR tries to tackle #7

The position added to the cells by CreateCaloCells uses volume ID + segmentation to calculate the position of the cells, but it lacks the z information (maybe the code could have worked but the part that retrieves the z information from the segmentation does not seem to be implemented, and would depend on the segmentation type).

Since we have already tools that translates cellID into position, I wrote a CreatePositionedCaloCells algorithm that works exactly like CreateCaloCells but for the cell position uses the positioning tool to calculate it and save it into the cell.

I think this tool could probably supersede both CreateCaloCells and CreateCaloCellPositionsFCCee, but for the moment I just added a new tool and left there the old ones so that nobody is affected.

The new tool is scheduled in the test being run automatically and is used systematically instead of CreateCaloCells and CreateCaloCellPositionsFCCee.

In this PR I also enabled in the test script the hcal endcap digisation and the new positioning tool for hcal barrel and endcap that were not included in the tests so far.

I will make some tests in the coming days to check that the output is unchanged by switching to the new tool but the review of the code structure can proceed in parallel.

@giovannimarchiori giovannimarchiori marked this pull request as draft July 24, 2024 10:06
@giovannimarchiori giovannimarchiori marked this pull request as ready for review August 13, 2024 12:09
@giovannimarchiori giovannimarchiori changed the title WIP: expanded version of CreateCaloCells that also add proper cell positions expanded version of CreateCaloCells that also add proper cell positions Aug 13, 2024
@kjvbrt
Copy link
Contributor

kjvbrt commented Sep 9, 2024

Hi @giovannimarchiori, would this algorithm work also for FCChh?
The question is mostly about the name for the algorithm...

@giovannimarchiori
Copy link
Contributor Author

giovannimarchiori commented Sep 9, 2024

Hi @kjvbrt - definitely. I guess you are suggesting to move the files to RecCalorimeter, like CreateCaloCells?

@kjvbrt
Copy link
Contributor

kjvbrt commented Sep 9, 2024

Yes


// Copy over the CellIDEncoding string from the input collection to the output collection
auto hitsEncoding = m_hitsCellIDEncoding.get_optional();
if (!hitsEncoding.has_value()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this worthy of crashing, can't this be just a warning?

Copy link
Contributor Author

@giovannimarchiori giovannimarchiori Sep 9, 2024

Choose a reason for hiding this comment

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

it's exactly as in CreateCaloCells (

// Copy over the CellIDEncoding string from the input collection to the output collection
). Without encoding the input cells can't be properly decoded in terms of layer and theta/eta/phi id which is needed for the various tools (positioning, calibration, ..). I would leave it like that, if you agree I will resolve the conversation

@giovannimarchiori
Copy link
Contributor Author

Hi @kjvbrt , I implemented your comments and answered one question. I reran the tests locally and also in GitHub and they ran fine

@kjvbrt
Copy link
Contributor

kjvbrt commented Sep 9, 2024

Hi @giovannimarchiori, nice :)
I'm merging this as it draws from the previous work, but I believe that ideally the algorithms like this need to be split into individual tools. And tools converted into algorithms...

@kjvbrt kjvbrt merged commit 54c0674 into HEP-FCC:main Sep 9, 2024
2 of 5 checks passed
@giovannimarchiori giovannimarchiori deleted the gmarchio-main-20240724-positionedcells branch September 9, 2024 19:42
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.

2 participants