Skip to content

Refactor pf coil placement#3840

Merged
timothy-nunn merged 20 commits intomainfrom
refactor_pf_coil_placement
Sep 25, 2025
Merged

Refactor pf coil placement#3840
timothy-nunn merged 20 commits intomainfrom
refactor_pf_coil_placement

Conversation

@chris-ashe
Copy link
Copy Markdown
Collaborator

@chris-ashe chris-ashe commented Sep 10, 2025

This pull request refactors how PF coil and TF coil positional parameters are named and handled throughout the codebase, improving clarity, consistency, and maintainability. It replaces ambiguous variable names with more descriptive ones, updates documentation to match the new conventions, and ensures all related modules, input handling, plotting, and tests use the updated parameters. Additionally, the documentation for PF coil placement logic is expanded and clarified.

🔄 Renames

rcls -> r_pf_coil_middle_group_array
zcls -> z_pf_coil_middle_group_array
rpf1 -> dr_pf_cs_middle_offset
routr -> dr_pf_tf_outboard_out_offset
rclsnorm -> r_pf_outside_tf_midplane
hpfdif -> dz_tf_upper_lower_midplane
i_sup_pf_shape -> i_r_pf_outside_tf_placement

🐛 Bugs

Wrong assumption between PF conductor type and location of PF coils. Is now based on coil shape

# Coil radius follows TF coil curve for SC TF (D-shape)
                    # otherwise stacked for resistive TF (rectangle-shape)
                    if tfv.i_tf_sup != 1 or pfcoil_variables.i_r_pf_outside_tf_placement == 1:
                        pfcoil_variables.r_pf_coil_middle_group_array[group, coil] = (
                            pfcoil_variables.r_pf_outside_tf_midplane
                        )

# Coil radius is constant / stacked for picture frame TF or if placement switch is set
                    if tfv.i_tf_shape == 2 or pfcoil_variables.i_r_pf_outside_tf_placement == 1:
                        pfcoil_variables.r_pf_coil_middle_group_array[group, coil] = (
                            pfcoil_variables.r_pf_outside_tf_midplane
                        )
                    else:

Documentation and logic updates:

  • Significantly expanded and clarified PF coil placement documentation, detailing the logic for all four possible values of i_pf_location(j) and the new/renamed variables, with explicit equations and placement routines.
  • Updated variable descriptor comments throughout to match new naming conventions and clarify their usage.

Plotting and visualization:

  • Added plotting and labeling of the new dz_tf_upper_lower_midplane parameter in TF coil structure plots for enhanced visualization.

Testing updates:

  • Updated integration tests to use the new variable names and ensure correct monkeypatching and test coverage for the refactored parameters.

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
Copy link
Copy Markdown

codecov-commenter commented Sep 10, 2025

Codecov Report

❌ Patch coverage is 39.39394% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.57%. Comparing base (be122e7) to head (2db8c96).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
process/pfcoil.py 31.25% 55 Missing ⚠️
process/build.py 0.00% 2 Missing ⚠️
process/io/plot_proc.py 0.00% 2 Missing ⚠️
process/tf_coil.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3840      +/-   ##
==========================================
- Coverage   46.57%   46.57%   -0.01%     
==========================================
  Files         122      123       +1     
  Lines       27898    28130     +232     
==========================================
+ Hits        12994    13102     +108     
- Misses      14904    15028     +124     

☔ View full report in Codecov by Sentry.
📢 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.

@chris-ashe chris-ashe force-pushed the refactor_pf_coil_placement branch 7 times, most recently from 863735d to 465d5ce Compare September 10, 2025 15:25
@timothy-nunn timothy-nunn force-pushed the refactor_pf_coil_placement branch from 09eda88 to 523471d Compare September 23, 2025 12:26
@chris-ashe chris-ashe marked this pull request as ready for review September 23, 2025 12:51
…e references throughout the PFCoil module and integration tests
…e references in PFCoil module and integration tests
…pdate references in Build and PFCoil classes and integration tests
…arity in TF coil height difference representation
@timothy-nunn timothy-nunn force-pushed the refactor_pf_coil_placement branch from 523471d to 1107d76 Compare September 23, 2025 14:28
Comment thread process/build.py
Comment thread process/input.py
Copy link
Copy Markdown
Collaborator

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

Choose a reason for hiding this comment

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

Happy with changes.

@timothy-nunn timothy-nunn merged commit 1d84ee8 into main Sep 25, 2025
14 of 18 checks passed
@timothy-nunn timothy-nunn deleted the refactor_pf_coil_placement branch September 25, 2025 09:45
grmtrkngtn pushed a commit that referenced this pull request Oct 10, 2025
* Refactor PF coil placement logic for improved readability and consistency

* Refactor PF coil placement logic by introducing a helper function for improved clarity and maintainability

* 🔄 - Rename rcls to r_pf_coil_middle_group_array for clarity and update references throughout the PFCoil module and integration tests

* 🔄 - Rename zcls to z_pf_coil_middle_group_array for clarity and update references in PFCoil module and integration tests

* 🔄 - Rename rpf1 to dr_pf_cs_middle_offset for clarity and update references in PFCoil module and integration tests

* 🔄 - Rename 'routr' to 'dr_pf_tf_outboard_out_offset' for clarity and update references in PF coil logic and tests

* 🔄 - Rename 'rclsnorm' to 'r_pf_outside_tf_midplane' for clarity and update references in PF coil logic and tests

* 🔄 - Refactor PF coil placement logic and add 'place_pf_above_tf' method for clarity and maintainability

* 🔄 - Rename 'hpfdif' to 'dz_tf_upper_lower_midplane' for clarity and update references in Build and PFCoil classes and integration tests

* 🐛 Fix calculated value of dz_tf_upper_lower_midplan

* 🎨 Add 'dz_tf_upper_lower_midplane' to plot and output for improved clarity in TF coil height difference representation

* 🔄 - Rename 'i_sup_pf_shape' to 'i_r_pf_outside_tf_placement' for clarity and update references in related files

* 🐛 Fix false association between TF coil shape and conductor type

* ♻️ Add new function for placing PF coils outside the TF

* ♻️ Refactor PF coil placement logic into a new method for improved clarity and maintainability

* 🎨 Update PF coil positioning documentation for clarity and accuracy, including new equations and descriptions for outside TF coil placement

* ♻️ Add parameterized test for placing PF coils above the CS with various configurations

* ♻️ Add unit tests for placing PF coils above TF with various configurations

* Update r/z_pf_coil_middle_group_array in the general case

* ♻️ Update obsolete variable mappings for PF coil placement offsets

---------

Co-authored-by: Timothy Nunn <timothy.nunn@ukaea.uk>
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