epic icon indicating copy to clipboard operation
epic copied to clipboard
verified publisher icon eic

Mirror ribs

Open chchatte92 opened this issue 2 years ago • 12 comments

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 avatar Jul 05 '23 15:07 chchatte92

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

chchatte92 avatar Jul 05 '23 16:07 chchatte92

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

wdconinc avatar Jul 05 '23 16:07 wdconinc

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

chchatte92 avatar Jul 05 '23 16:07 chchatte92

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

chchatte92 avatar Dec 13 '23 15:12 chchatte92

@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.

chchatte92 avatar Dec 13 '23 15:12 chchatte92

Looks good enough to me.

  • Are the optics the same? (besides the new gaps, of course)
  • Does this supersede #380 ?

c-dilks avatar Dec 13 '23 23:12 c-dilks

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 avatar Dec 14 '23 12:12 chchatte92

I'll defer to @c-dilks here

@c-dilks hahahaha

chchatte92 avatar Dec 15 '23 12:12 chchatte92

LGTM. Mark ready and add to merge queue.

c-dilks avatar Dec 16 '23 04:12 c-dilks

Please don't merge now!

chchatte92 avatar Dec 21 '23 16:12 chchatte92

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.

chchatte92 avatar Dec 21 '23 16:12 chchatte92

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

rahmans1 avatar Feb 07 '24 03:02 rahmans1