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

Mirror ribs #471

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Mirror ribs #471

wants to merge 16 commits into from

Conversation

chchatte92
Copy link
Member

@chchatte92 chchatte92 commented Jul 5, 2023 •
edited
Loading

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • [ X] Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • [X ] 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

@chchatte92 chchatte92 requested a review from c-dilks July 5, 2023 15:59
@github-actions github-actions bot added topic: PID Particle identification topic: materials labels Jul 5, 2023
@chchatte92
Copy link
Member Author

This is a first attempt to add some ribs in the mirror! Similar ribs to be added to the aerogel. Currently we have C2F6 rib, we will make it with carbon-fiber.
image

@wdconinc
Copy link
Contributor

wdconinc commented Jul 5, 2023

Use an updated base branch. Until you do, checks will fail.

@chchatte92
Copy link
Member Author

Hi @wdconinc thanks a lot! I just noticed it. I will take care of it.

@c-dilks c-dilks mentioned this pull request Aug 8, 2023
Closed
c-dilks
c-dilks reviewed Aug 8, 2023
View reviewed changes
src/DRICH_geo.cpp Outdated
Comment on lines 365 to 367
double mirrorTheta2 = 0.4*mirrorThetaRot + std::asin((mirrorRmax - mirrorCenterX) / mirrorRadius);
double mirrorTheta3 = 0.41*mirrorThetaRot + std::asin((mirrorRmax - mirrorCenterX) / mirrorRadius);
double mirrorTheta4 = 1.0*mirrorThetaRot + std::asin((mirrorRmax - mirrorCenterX) / mirrorRadius);
Copy link
Member

Choose a reason for hiding this comment

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

Move hard-coded numbers (0.4 and 0.41) to drich.xml.

src/DRICH_geo.cpp Outdated
Comment on lines 390 to 395
Tube pieSlice(0.01 * cm, vesselRmax2, tankLength / 2.0, -mirrorPhiw / 2.0, mirrorPhiw / 2.0);
Tube pieSlice1(0.01 * cm, vesselRmax2, tankLength / 2.0, -mirrorPhiw / 2.0, (0.05*mirrorPhiw) / 2.0);
Tube pieSlice2(0.01 * cm, vesselRmax2, tankLength / 2.0, (0.06*mirrorPhiw) / 2.0, mirrorPhiw / 2.0);
IntersectionSolid mirrorSolid2(pieSlice, mirrorSolid1, mirrorPlacement);
IntersectionSolid mirrorSolid4(pieSlice1, mirrorSolid3, mirrorPlacement);
IntersectionSolid mirrorSolid5(pieSlice2, mirrorSolid3, mirrorPlacement);
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it's time to come up with clearer names than using incrementing numbers, since we are now at "5".

Copy link
Member

Choose a reason for hiding this comment

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

I don't have any good ideas, but maybe something like

  • mirrorSolidInnerTile
  • mirrorSolidOuterTileA
  • mirrorSolidOuterTileB

Or you could put these in some STL container, such as std::set, so that you don't really have to give them names.

src/DRICH_geo.cpp Outdated
Comment on lines 398 to 400
Volume mirrorVol(detName + "_mirror_0" + secName, mirrorSolid2, mirrorMat);
Volume mirrorVol2(detName + "_mirror_1" + secName, mirrorSolid4, mirrorMat);
Volume mirrorVol3(detName + "_mirror_2" + secName, mirrorSolid5, mirrorMat);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Volume mirrorVol(detName + "_mirror_0" + secName, mirrorSolid2, mirrorMat);
Volume mirrorVol2(detName + "_mirror_1" + secName, mirrorSolid4, mirrorMat);
Volume mirrorVol3(detName + "_mirror_2" + secName, mirrorSolid5, mirrorMat);
Volume mirrorVol(detName + "_mirror_tile0" + secName, mirrorSolid2, mirrorMat);
Volume mirrorVol2(detName + "_mirror_tile1" + secName, mirrorSolid4, mirrorMat);
Volume mirrorVol3(detName + "_mirror_tile2" + secName, mirrorSolid5, mirrorMat);

or some suitable name, just so it's clear what 0, 1, and 2 mean.

@github-actions github-actions bot added the topic: forward Positive-rapidity detectors (hadron-going side) label Dec 13, 2023
@chchatte92
Copy link
Member Author

@c-dilks I have changed the variables names to something more meaningful and have also added a set of ribs.

@chchatte92
Copy link
Member Author

@c-dilks I have changed the variables names to something more meaningful and have also added a set of ribs.

This is how it looks now:
image
The rib material is now Aluminium. I was wandering to make it in future to carbon fibre.

@c-dilks
Copy link
Member

c-dilks commented Dec 13, 2023 •
edited
Loading

Looks good enough to me.

@chchatte92
Copy link
Member Author

Looks good enough to me.

Nice

* Are the optics the same? (besides the new gaps, of course)

The optics are same. I have just reduced the sensor phi cut from 30 degrees to 18 degrees. I guess 30 degrees are overdoing.

* Does this supersede [Dual mirror implementation #380](https://github.com/eic/epic/pull/380) ?

The mirrors are of single radius. Actually, this I with like to discuss sometime with you. Maybe we can ascribe different radius and centres directly from .xml file. But that means IRT has to be upgraded to deal with two radii.

@chchatte92 chchatte92 requested a review from wdconinc December 14, 2023 12:56
wdconinc
wdconinc reviewed Dec 14, 2023
View reviewed changes
Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

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

I'll defer to @c-dilks here

@chchatte92
Copy link
Member Author

I'll defer to @c-dilks here

@c-dilks hahahaha

@c-dilks
Copy link
Member

c-dilks commented Dec 16, 2023

LGTM. Mark ready and add to merge queue.

@chchatte92 chchatte92 marked this pull request as ready for review December 21, 2023 13:39
@chchatte92
Copy link
Member Author

Please don't merge now!

@chchatte92 chchatte92 marked this pull request as draft December 21, 2023 16:13
@chchatte92
Copy link
Member Author

LGTM. Mark ready and add to merge queue.

@c-dilks I had a discussion with Macro today, and he is right. That this gap should not be carbon fibre but C2F6, as these open region is useful for alignment of the mirror. This aligns with my first implementation. I will change the rib material.

@rahmans1
Copy link
Contributor

rahmans1 commented Feb 7, 2024

@chchatte92 What is the status of this PR? Should it be included in the February release for simulation campaign?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wdconinc wdconinc wdconinc left review comments

@c-dilks c-dilks c-dilks approved these changes

Assignees

@chchatte92 chchatte92

Labels
topic: forward Positive-rapidity detectors (hadron-going side) topic: PID Particle identification
Projects
dRICH Simulations
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chchatte92 @wdconinc @c-dilks @rahmans1