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

Improved electron beam pipe modelling #691

Closed
wants to merge 46 commits into from
Closed

Conversation

nat93
Copy link

@nat93 nat93 commented Mar 29, 2024

Briefly, what does this PR introduce?

It contains the improved geometry of the central and far-backward beam pipes. In addition, some overlaps between ePIC and the new beam pipe are fixed by changing the detector components' inner radius (this has to be approved by detector experts).

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 (could be checked by plotting the entire geometry, e.g., through 'dd_web_display,' or material scan, e.g., through 'g4MaterialScan_to_csv')
  • Documentation has been added/updated (comments were added)
  • Changes have been communicated to collaborators (partially, the Vacuum and Background groups approved the changes)

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

We must check how the modified subsystem geometries affect detector performance (angular acceptance, resolution, etc.).

Does this PR change default behavior?

Yes. Now, we should have a more accurate beam pipe with an enlarged inner radius of subsystems in the forward direction to fit the new beam pipe. This may affect detector background rates.

Things to be done

Still, the materials of the beam pipes (by default, aluminum) are wrong, except for the IP beam pipe. The next step would be to fix this issue (it should be stainless steel with a copper coating).

References

Machine lattice file v6.2: esr-ring-norad.tfs and esr-survey-doug.tfs
Electron beam pipe vacuum geometry (from the EIC vacuum group): detector_chamber_211004.pdf and STL file
Detector geometry file - Apr 2024: EPIC Envelope - 04-03-2024.pdf
Updated detector geometry table - Apr 2024: Detector-20240426

@github-actions github-actions bot added topic: infrastructure Regarding build system, CI, CD topic: tracking topic: far-forward Deterctors for small-angle particles topic: far-backward topic: magnets labels Mar 29, 2024
@veprbl veprbl force-pushed the eic_sr_background_study branch from 9a507a8 to 490c56c Compare April 1, 2024 06:34
<id>system:8</id>
</readout>
</readouts>

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is supposed to go into the official geometry. Why do we need this?

Copy link
Author

@nat93 nat93 Apr 1, 2024

Choose a reason for hiding this comment

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

Hi @veprbl ,
Thanks for your comment. The motivation for making the luminosity monitor exit window sensitive to radiation is that we have to study this element (a simple aluminum block) and check if it will survive under a high flux of synchrotron radiation photons. So far, the preliminary study showed that at 18 GeV, 0.227 A electron beam, the window will melt in 2 hours. So, we have to think about additional cooling. Therefore, I would suggest keeping this part of the code to collect deposited energy in that element.
What do you think?

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.

Thanks for this PR. That's a lot of great work that we should work to include in the epic geometry for one of the next campaigns. I have left some comments. Mainly, please coordinate with @ajentsch in the context of #665 to make sure there is no duplication of efforts.

@@ -144,7 +144,7 @@ static Ref_t create_detector(Detector& det, xml_h e, SensitiveDetector /* sens *
// -->the volume will be calculated at the end
//-------------------------------------------

double endOfCentralBeamPipe_z = 445.580*dd4hep::cm; //extracted from central_beampipe.xml, line 64
double endOfCentralBeamPipe_z = 560.00*dd4hep::cm; // <-- extracted from central_beampipe.xml
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get this from the geometry instead of copying the value and hardcoding it? (I know, not your fault it was already hardcoded, but might as well fix it while we're at it. Also, maybe this should be done in #665.)

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid overlap with what @ajentsch has already done in that PR #665, please coordinate with him.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be fine. That's the one part of my magnet volume code which is done that way since we didn't have the numbers somewhere to be grabbed dynamically. @nat93 can you add the change to magnetVacuumFF.cpp to grab the number from line 147 from the central_beampipe.xml line 43?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @ajentsch and @wdconinc , thanks for your comments. Yes, I will fix it, following Alex's suggestion.

Comment on lines 16 to 19
<pipe id="0" name="Pipe_to_Q1eR"
xcenter="0" zcenter="(Center_Beampipe_End + Q1eR_CenterPosition+Q1eR_Length/2)/2"
length="Center_Beampipe_End - (Q1eR_CenterPosition+Q1eR_Length/2)" theta="0"
rout1="Center_Beampipe_Rad" rout2="Q1eR_InnerRadius">
<pipe id="0" name="Pipe_0"
xcenter="0" zcenter="-7342.0 * mm + (Center_Beampipe_End + 7342.0 * mm)/2"
length="Center_Beampipe_End + 7342.0 * mm" theta="0"
rout1="Center_Beampipe_Rad" rout2="(111.40/2 + 2) * mm">
Copy link
Contributor

Choose a reason for hiding this comment

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

We should avoid changing parametrized values into hard-coded values that are then used in multiple places. (Applies file-wide.)

Copy link
Author

Choose a reason for hiding this comment

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

It is true. However, I would suggest keeping the beam pipe geometry separated from the magnet geometry because they have different sources (i.e., the beam pipe model is from the vacuum and background groups, magnets -- lattice and magnet groups). Also, the inner radius of magnets should not always follow the beam pipe radius.
I can create beam pipe global variables in the central-beam pipe XML file to be used here. What do you think @wdconinc ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if we need them all as global variables in the central beam pipe; they can be local variables in the far_backward and far_forward sections, right?

Comment on lines +26 to +27
<constant name="B2BeR_Length" value="5.500075391030 * m"/>
<constant name="B2BeR_CenterPosition" value="-15.000075390000 * m + B2BeR_Length/2"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the microns matter here?

Suggested change
<constant name="B2BeR_Length" value="5.500075391030 * m"/>
<constant name="B2BeR_CenterPosition" value="-15.000075390000 * m + B2BeR_Length/2"/>
<constant name="B2BeR_Length" value="5.5 * m"/>
<constant name="B2BeR_CenterPosition" value="-15.0 * m + B2BeR_Length/2"/>

Copy link
Author

Choose a reason for hiding this comment

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

Here, I simply follow the lattice file v6.2.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are following a specific lattice version, we should mention this in one of the comments somewhere, with a reference for where one can find this lattice version (even if not publicly accessible).

Comment on lines -458 to +459
<constant name="ForwardRICHRegion_tan1" value="CentralTrackingRegionP_tan * 0.88" />
<constant name="ForwardRICHRegion_tan2" value="Eta3_6_tan * 0.89" />
<constant name="ForwardRICHRegion_tan1" value="CentralTrackingRegionP_tan * 1.20" />
<constant name="ForwardRICHRegion_tan2" value="Eta3_6_tan * 1.20" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the shape of the dRICH and can't get merged without TIC review and approval.

Copy link
Author

Choose a reason for hiding this comment

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

Could you please ping the relevant experts? Because we cannot use the actual beam pipe due to overlaps with dRICH.

Comment on lines 55 to 71
<zplane z="BeampipeDownstreamStraightLength" OD="BeampipeOD"/>
<zplane z="4484.25 * mm" OD="BeampipeOD"/>
<zplane_vac z="670.00 * mm" OD="60.00 * mm"/>
<zplane_vac z="755.00 * mm" OD="60.00 * mm"/>
<zplane_vac z="755.00 * mm" OD="62.00 * mm"/>
<zplane_vac z="4300.00 * mm" OD="62.00 * mm"/>
<zplane_vac z="4450.00 * mm" OD="44.00 * mm"/>
<zplane_vac z="4460.00 * mm" OD="44.00 * mm"/>
<zplane_vac z="4660.00 * mm" OD="54.00 * mm"/>
<zplane_vac z="5100.00 * mm" OD="54.00 * mm"/>

<zplane_mat z="670.00 * mm" OD="(62.00 + 1.0 * 2) * mm"/>
<zplane_mat z="(670.00 + 1.0) * mm" OD="(62.00 + 1.0 * 2) * mm"/>
<zplane_mat z="(670.00 + 1.0) * mm" OD="(60.00 + 1.0 * 2) * mm"/>
<zplane_mat z="(755.00 - 1.0) * mm" OD="(60.00 + 1.0 * 2) * mm"/>
<zplane_mat z="(755.00 - 1.0) * mm" OD="(62.00 + 1.0 * 2) * mm"/>
<zplane_mat z="4300.00 * mm" OD="(62.00 + 1.0 * 2) * mm"/>
<zplane_mat z="4450.00 * mm" OD="(44.00 + 1.0 * 2) * mm"/>
<zplane_mat z="4460.00 * mm" OD="(44.00 + 1.0 * 2) * mm"/>
<zplane_mat z="4660.00 * mm" OD="(54.00 + 1.0 * 2) * mm"/>
<zplane_mat z="5100.00 * mm" OD="(54.00 + 1.0 * 2) * mm"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like the duplication of all z and many of the OD values here. Can we approach this conceptually different so it doesn't require this level of duplication?

Copy link
Contributor

Choose a reason for hiding this comment

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

This now also adds another methodology to define beampipes: we can still use <zplane> but now we have <zplane_mat> and <zplane_vac>. We may want to have one way, which sensibly defaults back to the previous behavior.

Copy link
Author

Choose a reason for hiding this comment

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

OK. Thanks. I will think about this.

compact/far_backward/magnets.xml Show resolved Hide resolved
@@ -201,6 +233,121 @@ static Ref_t create_detector(Detector& det, xml_h e, SensitiveDetector /* sens *
return std::make_pair<Volume, Volume>({"v_" + name + "_matter", matter, m_Al},
{"v_" + name + "_vacuum", vacuum, m_Vacuum});
};
//---------------------------------------------------------------------------------
// Create downstream volumes
auto create_volumes_2 = [&](const std::string& name, xml::Component& x_pipe1, xml::Component& x_pipe2,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not duplicate the API here, but make the original one work reliably in both use cases. This code duplication doubles maintenance burden.

Copy link
Author

Choose a reason for hiding this comment

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

OK. I will try to merge them.

// _________/......
// ................

auto zplane_to_polycones_2 = [](xml::Component& x_pipe) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not duplicate the API here, but make the original one work reliably in both use cases. This code duplication doubles maintenance burden.

Copy link
Author

Choose a reason for hiding this comment

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

OK. I will try to merge them.

<constant name="ForwardTOFRegion_minR" value="8*cm" />
<constant name="ForwardTOFRegion_minR" value="10*cm" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes to detector envelope volumes require updates to the detector parameter table first, along with TIC review and approval (https://eic.jlab.org/Geometry/Detector/Detector-20240117135224.html indicates 8 cm).

Copy link
Author

Choose a reason for hiding this comment

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

As I wrote, we cannot fit the actual beam pipe in the current detector. Please let me know the procedure to get it approved. Do you know who should be contacted?

Copy link
Contributor

Choose a reason for hiding this comment

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

The current procedure is to send an email with requests for changes to the detector parameter table to the detector parameter table maintainers, the project's experiment contacts, collaboration leadership, and the specific detector contacts. I will send you their emails instead of posting them here.

Comment on lines 105 to 106
<constant name="ForwardMPGDMod1_rmin" value="7.014*cm" />
<constant name="ForwardMPGDMod2_rmin" value="7.014*cm"/>
<constant name="ForwardMPGDMod1_rmin" value="8.014*cm" />
<constant name="ForwardMPGDMod2_rmin" value="9.014*cm"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes to detector envelope volumes require updates to the detector parameter table first, along with TIC review and approval.

natochii and others added 22 commits April 2, 2024 07:46
…c_sr_background_study

integrate the remote changes
…c_sr_background_study

Merge from git before push
@wdconinc
Copy link
Contributor

wdconinc commented May 7, 2024

Formatted, merged main, conflicts resolved in favor of feature branch.

@nat93 nat93 closed this Sep 11, 2024
@veprbl veprbl reopened this Sep 11, 2024
@veprbl veprbl added this to the 24.09.0 milestone Sep 11, 2024
@nat93 nat93 closed this Sep 11, 2024
@veprbl veprbl removed this from the 24.09.0 milestone Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants