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

Compact lumi pair spectrometer #498

Merged
merged 12 commits into from
Sep 3, 2023
Merged

Compact lumi pair spectrometer #498

merged 12 commits into from
Sep 3, 2023

Conversation

dhevang
Copy link
Contributor

@dhevang dhevang commented Aug 10, 2023

Briefly, what does this PR introduce?

Implement a new luminosity dipole magnet based on design from BNL engineer Peng Xu.
The stronger magnet allows the pair spectrometer to be much more compact.

What kind of change does this PR introduce?

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

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

wdconinc
wdconinc previously approved these changes Aug 10, 2023
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.

This looks like a lot of nice work.

@dhevang
Copy link
Contributor Author

dhevang commented Aug 11, 2023

Made commits to fix an overlap error in my system and for Dima's copyright suggestion. However, now another overlap error occurs in epic_craterlake. It appears to be related to some change in the beamline magnet volumes.

@wdconinc
Copy link
Contributor

It appears to be related to some change in the beamline magnet volumes.

That's why I asked @ajentsch to double check with you that his beamline magnet changes wouldn't affect the backward region :-) There were no overlaps after those changes were merged in main.

@ajentsch
Copy link
Contributor

It appears to be related to some change in the beamline magnet volumes.

That's why I asked @ajentsch to double check with you that his beamline magnet changes wouldn't affect the backward region :-) There were no overlaps after those changes were merged in main.

How could my changes cause overlaps on the opposite side of ePIC?? (I did communicate the changes to both @dhevang and @simonge)

@dhevang
Copy link
Contributor Author

dhevang commented Aug 11, 2023

Yeah, Alex mentioned the change to me.
The tests that I triggered 18 hours ago went through fine without this overlap error. The one I triggered this morning had a problem though. All I did in between was change a copyright comment.

@wdconinc
Copy link
Contributor

How could my changes cause overlaps on the opposite side of ePIC?

Because you changed a magnet geometry description that is also used on the opposite side of ePIC.

The tests that I triggered 18 hours ago went through fine without this overlap error.

Not getting an overlap error is not a guarantee that there are no overlaps (but getting an overlap error is a guarantee that there is an overlap):

  • overlap checking is a statistical procedure, not an analytic procedure,
  • there is a tolerance, and for overlaps that are close to the tolerance you may 'sneak through' without triggering an overlap error.

@ajentsch
Copy link
Contributor

ajentsch commented Aug 11, 2023 via email

@dhevang
Copy link
Contributor Author

dhevang commented Aug 18, 2023

Well, the last 2 CI tests that I triggered went through fine (no FF overlap error). Despite the statistical limitation to these checks, should we merge this into main or wait till a fix comes? It should affect all other PRs too right?

@ajentsch
Copy link
Contributor

ajentsch commented Aug 18, 2023 via email

@dhevang
Copy link
Contributor Author

dhevang commented Sep 1, 2023

@wdconinc initiated two new tests today and both failed with the same overlap error:
Overlap is detected for volume AV_11293!VacuumMagnetElement_assembly_51#51!v_b1pf_vacuum_5#5:57 (G4Cons) with AV_11287!Q2PF_BeamlineMagnet_assembly_29#29!yoke_vol_0#0:30 (G4Tubs)
Concerning the issue, I suppose the problem is that the G4Tubs do not lie on the same straight line, because there's the b1pf dipole. The vacuum tube inside the b1pf is at a slight angle wrt the Q2PF tube. So a tiny corner of the vacuum tube edge probably hits the Q2PF. Although not the most elegant solution, could one simply modify the length or radius of the vacuum by like 0.5 mm? @ajentsch is that a viable solution?

@wdconinc wdconinc merged commit dccb089 into main Sep 3, 2023
50 of 70 checks passed
@wdconinc wdconinc deleted the CompactLumiPS branch September 3, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants