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

STT default geometry added #26

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

nibir98
Copy link
Member

@nibir98 nibir98 commented Jul 11, 2024

The default STT geometry for SAND is added. The build_hall.sh script is also updated to build the geometries.

Copy link
Member

@ast0815 ast0815 left a comment

Choose a reason for hiding this comment

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

If this is the new default SAND geometry, I assume we should be using it from here on out? Then it should be added to the "prod" geometries (the first two in the build script). These are the ones that get built by default and released.

Also, you need to add your changes to the "Unreleased" section of the CHANGELOG file.

@nibir98
Copy link
Member Author

nibir98 commented Jul 12, 2024

If this is the new default SAND geometry, I assume we should be using it from here on out? Then it should be added to the "prod" geometries (the first two in the build script). These are the ones that get built by default and released.

Also, you need to add your changes to the "Unreleased" section of the CHANGELOG file.

If you want to build SAND using STT, yes this is the default geometry. I assume all the miniprods till now has built SAND using STT? If so pls, let me know I can make the required changes

Copy link
Contributor

Choose a reason for hiding this comment

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

why this option, supposed to be the default one, forsees a diffent clearances among ECAL GRAIN and the Tracker?

Copy link
Member Author

Choose a reason for hiding this comment

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

The grain size has been updated to 902mm. The clearances are listed in the file are just guessing parameters. Here are the final parameters.
GRAIN-ECAL | 1.15 centimeter
GRAIN-STT | 11.568251845746317 millimeter
STT-ECAL | 0.0764459008996361 meter
The total STT length is 2998.4 mm. Details are included in page 11 here

@nibir98
Copy link
Member Author

nibir98 commented Jul 15, 2024

If this is the new default SAND geometry, I assume we should be using it from here on out? Then it should be added to the "prod" geometries (the first two in the build script). These are the ones that get built by default and released.

Also, you need to add your changes to the "Unreleased" section of the CHANGELOG file.

The changes has been implemented as requested.

Copy link
Member

@ast0815 ast0815 left a comment

Choose a reason for hiding this comment

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

Looks like you actually forgot to commit the changes to the changelog.

@ast0815
Copy link
Member

ast0815 commented Sep 5, 2024

Hey, what's the status here?

@nibir98
Copy link
Member Author

nibir98 commented Sep 5, 2024

Hi Lukas, sorry for the delay. I talked to the SAND software group yesterday because the reco/ana codes for SAND might break with the new geometry as it contains some new volume names. We think that the we should release the new SAND geometry as a standalone instead of integrating it with the prod geometries for the moment. I will work in parallel towards the integration, and once that's done we can add it with prod.
P.S There are 2 new STT geometries (default & backup) which exist. I will release a PR by the end of this week having the 2 geometries as standalone. What do you think?
In the future we will integrate the default version with prod.

@ast0815
Copy link
Member

ast0815 commented Sep 9, 2024

I am perfectly happy if you want to keep the new geometry as an option rather than as the default. As long as it gets merged into the master branch, I am content. The rest is up for other people to decide. Will you edit this PR or will you create a new one? In the latter case we could close this one.

P.S. Don't forget the changelog.

@nibir98
Copy link
Member Author

nibir98 commented Sep 10, 2024

Hi Lukas, I will create a fresh PR. Thanks.

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.

3 participants