Skip to content

Add specific dr_cs_bore variable#4437

Open
chris-ashe wants to merge 9 commits into
mainfrom
add_specific_dr_cs_bore_var
Open

Add specific dr_cs_bore variable#4437
chris-ashe wants to merge 9 commits into
mainfrom
add_specific_dr_cs_bore_var

Conversation

@chris-ashe

@chris-ashe chris-ashe commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

This pull request refactors the codebase to consistently use a new variable, dr_cs_bore, to represent the central solenoid (CS) bore radius, replacing the previous use of dr_bore for this purpose. The changes improve clarity and accuracy in the modeling of the CS geometry, update all related calculations, plotting routines, and tests, and provide more precise documentation for these variables.

Variable refactoring and improved clarity:

  • Introduced dr_cs_bore in BuildData to explicitly represent the CS bore radius, updated its documentation, and ensured it is set appropriately in the radial build calculations (process/data_structure/build_variables.py, process/models/build.py).
  • Refactored all geometry, plotting, and collision detection code to use dr_cs_bore instead of dr_bore when referring to the CS bore, including function signatures, calculations, and docstrings (process/models/geometry/pfcoil.py, process/models/pfcoil.py, process/core/io/plot/summary.py).
  • Updated all output and documentation strings to reference dr_cs_bore where appropriate (process/models/pfcoil.py).

Testing updates:

  • Modified all relevant tests to use dr_cs_bore instead of dr_bore for the CS bore radius, ensuring test consistency with the new variable naming and logic (tests/unit/models/test_pfcoil.py).

Plotting improvements:

  • Adjusted text placement and axis values in plotting routines for improved visualization and to reflect the variable name changes (process/core/io/plot/summary.py).

These changes make the codebase more maintainable and reduce ambiguity around the meaning of the bore radius variables, ensuring that the CS bore and other bore-related quantities are clearly distinguished.## Description

Checklist

I confirm that I have completed the following checks:

  • My changes follow the PROCESS style guide
  • I have justified any large differences in the regression tests caused by this pull request in the comments.
  • I have added new tests where appropriate for the changes I have made.
  • If I have had to change any existing unit or integration tests, I have justified this change in the pull request comments.
  • If I have made documentation changes, I have checked they render correctly.
  • I have added documentation for my change, if appropriate.

@codecov-commenter

codecov-commenter commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 19.04762% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.64%. Comparing base (9333159) to head (8fb90bc).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
process/core/io/plot/summary.py 0.00% 10 Missing ⚠️
process/models/build.py 0.00% 7 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4437   +/-   ##
=======================================
  Coverage   48.64%   48.64%           
=======================================
  Files         151      151           
  Lines       29673    29673           
=======================================
+ Hits        14433    14434    +1     
+ Misses      15240    15239    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@timothy-nunn timothy-nunn self-assigned this Jul 13, 2026
@timothy-nunn

Copy link
Copy Markdown
Collaborator

Given the logic change in build.py I'm going to request @j-a-foster to review too

@timothy-nunn
timothy-nunn requested a review from j-a-foster July 14, 2026 08:27
Comment thread process/models/pfcoil.py

@j-a-foster j-a-foster left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Happy with changes.

Comment thread process/models/build.py Outdated
+ 2.0 * self.data.build.dr_cs_tf_gap
+ self.data.build.dr_cs
)
* (2.0 * self.data.build.dr_cs_bore + self.data.build.dr_cs)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is dr_cs_bore being used here before it is set?

@chris-ashe
chris-ashe requested a review from timothy-nunn July 14, 2026 14:46
Comment thread process/models/build.py Outdated
@@ -1723,6 +1687,22 @@ def calculate_radial_build(self, output: bool):
+ self.data.build.dr_cs_precomp

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

dr_cs_precomp is being used here but calculated below!

@timothy-nunn
timothy-nunn requested a review from j-a-foster July 16, 2026 09:26
@timothy-nunn

Copy link
Copy Markdown
Collaborator

@j-a-foster sorry to ask for another, could you just check the re-ordering in build.py to ensure I haven't made a mistake

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