Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4152 +/- ##
==========================================
- Coverage 48.02% 47.98% -0.05%
==========================================
Files 143 144 +1
Lines 30088 30334 +246
==========================================
+ Hits 14451 14555 +104
- Misses 15637 15779 +142 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ingTFCoil - Added fixtures for CICCSuperconductingTFCoil and CROCOSuperconductingTFCoil. - Updated test cases to utilize cicc_sctfcoil fixture instead of the generic sctfcoil. - Ensured that all relevant tests are now using the specific coil types for improved accuracy.
…P and casing areas
…ld calculation with ripple
…COSuperconductingTFCoil classes
chris-ashe
force-pushed
the
create_specific_turn_tf_classes
branch
from
3473018 to
b023ab3
Compare
je-cook
left a comment
je-cook
left a comment
There was a problem hiding this comment.
I'm not a huge fan of the large number of float returns in this. It would be preferable to return a dataclass so the variable names are propagated and hard coded so most variable name order issues are avoided eg superconducting_tf_case_geometry and others. Obviously this would need to be then fed through. Rough metric >= 5 float outputs probably should be dataclass.
| output: bool | ||
|
|
||
| """ | ||
| self.iprint = 0 |
There was a problem hiding this comment.
I dont think iprint does anything anymore
| def __init__(self): | ||
| super().__init__() |
There was a problem hiding this comment.
This does nothing
| def __init__(self): | |
| super().__init__() |
| ) | ||
| == SuperconductingTFTurnType.CABLE_IN_CONDUIT | ||
| ): | ||
| self.models.cicc_sctfcoil.run() |
There was a problem hiding this comment.
I dont know whether this should be organised differently in future eg self.models.sctfcoil.cicc.run().
this if check would then be moved into some sctfcoil object. Right know I'm not going to hold this up but before we add a future cable design we have to reoganise this. I'm not yet sure what that will look like.
There was a problem hiding this comment.
Pull request overview
This PR refactors the superconducting TF coil workflow to support multiple turn geometry types by introducing turn-type-specific model classes and routing execution/output based on a new i_tf_turn_type input.
Changes:
- Added
SuperconductingTFTurnTypeand new TF coil model classes (CICCSuperconductingTFCoil,CROCOSuperconductingTFCoil) and updated the main model registry. - Added
i_tf_turn_typeto the data structure, input handling, and TF output. - Updated unit tests to use the new model classes and adapted a geometry helper to return a dataclass.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
process/models/tfcoil/superconducting.py |
Introduces turn-type enum, geometry dataclass, and new per-turn-type model classes; refactors common setup. |
process/core/caller.py |
Selects which TF superconducting model to run based on i_tf_turn_type. |
process/core/output.py |
Selects which TF superconducting model to output based on i_tf_turn_type. |
process/core/input.py |
Registers new input variable i_tf_turn_type. |
process/data_structure/superconducting_tf_coil_variables.py |
Adds and initializes i_tf_turn_type. |
process/models/tfcoil/base.py |
Adds i_tf_turn_type to TF output block. |
process/main.py |
Instantiates and stores the new TF superconducting model classes. |
tests/unit/test_sctfcoil.py |
Adds fixtures for new model classes and updates tests for the refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.superconducting_tf_coil_areas_and_masses() | ||
| def output(self): | ||
| self.outtf() | ||
| self.run_base_superconducting_tf() |
There was a problem hiding this comment.
SuperconductingTFCoil.output() calls run_base_superconducting_tf() directly, which bypasses subclass run() implementations (CICCSuperconductingTFCoil.run / CROCOSuperconductingTFCoil.run). This means turn-geometry + superconductor-property calculations may never run before output/stress, leading to stale/None values in the output. Consider making output() call self.run(output=True) (polymorphic) and keeping shared setup inside run_base_superconducting_tf().
| self.run_base_superconducting_tf() | |
| self.run(output=True) |
| b_tf_inboard_peak: {b_tf_inboard_peak} | ||
| j_superconductor: {j_superconductor} | ||
| """ | ||
| ) |
There was a problem hiding this comment.
calculate_superconductor_temperature_margin() returns temp_tf_superconductor_margin, but that variable is only assigned inside the if i_tf_superconductor in (...) block. For other values (e.g. 2 or 6) this will raise UnboundLocalError at return. Add an else path (or initialize a default) and/or raise a clear error for unsupported conductor types.
| ) | |
| ) | |
| else: | |
| raise ProcessValueError( | |
| f"Unsupported TF superconductor type for temperature margin " | |
| f"calculation: i_tf_superconductor={i_tf_superconductor}" | |
| ) |
| if temp_tf_superconductor_margin <= 0.0e0: | ||
| logger.error( | ||
| """Negative TFC temperature margin | ||
| temp_tf_superconductor_margin: {temp_tf_superconductor_margin} | ||
| b_tf_inboard_peak: {b_tf_inboard_peak} | ||
| j_superconductor: {j_superconductor} | ||
| """ | ||
| ) |
There was a problem hiding this comment.
This error log string is missing an f prefix, so {temp_tf_superconductor_margin}, {b_tf_inboard_peak}, and {j_superconductor} will not be interpolated and the message will be misleading. Make this an f-string (or use structured logging args).
| tfcoil_variables.temp_tf_superconductor_margin = self.calculate_superconductor_temperature_margin( | ||
| i_tf_superconductor=tfcoil_variables.i_tf_sc_mat, | ||
| j_superconductor=superconducting_tf_coil_variables.j_tf_superconductor, | ||
| b_tf_inboard_peak=tfcoil_variables.b_tf_inboard_peak_with_ripple, | ||
| strain=strain, | ||
| bc20m=superconducting_tf_coil_variables.b_tf_superconductor_critical_zero_temp_strain, | ||
| tc0m=superconducting_tf_coil_variables.temp_tf_superconductor_critical_zero_field_strain, | ||
| c0=1.0e10, | ||
| temp_tf_coolant_peak_field=tfcoil_variables.tftmp, | ||
| ) |
There was a problem hiding this comment.
In CROCOSuperconductingTFCoil.run(), calculate_superconductor_temperature_margin() is called unconditionally using superconducting_tf_coil_variables.j_tf_superconductor / critical-surface values, but those are not set by the CroCo path (supercon_croco) and may still be None. This can crash (and also conflicts with the fact CroCo already computes tmarg). Consider skipping this call for CROCO_REBCO and/or setting the required variables in supercon_croco.
| tfcoil_variables.temp_tf_superconductor_margin = self.calculate_superconductor_temperature_margin( | |
| i_tf_superconductor=tfcoil_variables.i_tf_sc_mat, | |
| j_superconductor=superconducting_tf_coil_variables.j_tf_superconductor, | |
| b_tf_inboard_peak=tfcoil_variables.b_tf_inboard_peak_with_ripple, | |
| strain=strain, | |
| bc20m=superconducting_tf_coil_variables.b_tf_superconductor_critical_zero_temp_strain, | |
| tc0m=superconducting_tf_coil_variables.temp_tf_superconductor_critical_zero_field_strain, | |
| c0=1.0e10, | |
| temp_tf_coolant_peak_field=tfcoil_variables.tftmp, | |
| ) | |
| if ( | |
| SuperconductorModel(tfcoil_variables.i_tf_sc_mat) | |
| != SuperconductorModel.CROCO_REBCO | |
| ): | |
| tfcoil_variables.temp_tf_superconductor_margin = self.calculate_superconductor_temperature_margin( | |
| i_tf_superconductor=tfcoil_variables.i_tf_sc_mat, | |
| j_superconductor=superconducting_tf_coil_variables.j_tf_superconductor, | |
| b_tf_inboard_peak=tfcoil_variables.b_tf_inboard_peak_with_ripple, | |
| strain=strain, | |
| bc20m=superconducting_tf_coil_variables.b_tf_superconductor_critical_zero_temp_strain, | |
| tc0m=superconducting_tf_coil_variables.temp_tf_superconductor_critical_zero_field_strain, | |
| c0=1.0e10, | |
| temp_tf_coolant_peak_field=tfcoil_variables.tftmp, | |
| ) |
| != SuperconductorMaterial.NB3SN | ||
| ): | ||
| logger.warning( | ||
| "Calculating current density protection limit for Nb3Sn TF coil (LTS windings only)" | ||
| ) | ||
| # Find the current density limited by the protection limit | ||
| # At present only valid for LTS windings (Nb3Sn properties assumed) | ||
| tfcoil_variables.j_tf_wp_quench_heat_max, v_tf_coil_dump_quench = ( | ||
| self.quench_heat_protection_current_density( | ||
| c_tf_turn=tfcoil_variables.c_tf_turn, | ||
| e_tf_coil_magnetic_stored=tfcoil_variables.e_tf_coil_magnetic_stored, | ||
| a_tf_turn_cable_space=tfcoil_variables.a_tf_turn_cable_space_no_void, | ||
| a_tf_turn=a_tf_turn, | ||
| t_tf_quench_dump=tfcoil_variables.t_tf_superconductor_quench, | ||
| f_a_tf_turn_cable_space_conductor=1.0e0 | ||
| - superconducting_tf_coil_variables.f_a_tf_turn_cable_space_cooling, | ||
| f_a_tf_turn_cable_copper=tfcoil_variables.f_a_tf_turn_cable_copper, | ||
| temp_tf_coolant_peak_field=tfcoil_variables.tftmp, | ||
| temp_tf_conductor_quench_max=tfcoil_variables.temp_tf_conductor_quench_max, | ||
| b_tf_inboard_peak=tfcoil_variables.b_tf_inboard_peak_with_ripple, | ||
| cu_rrr=tfcoil_variables.rrr_tf_cu, | ||
| t_tf_quench_detection=tfcoil_variables.t_tf_quench_detection, | ||
| nflutfmax=constraint_variables.nflutfmax, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
CROCOSuperconductingTFCoil.run() always performs the quench protection current-density calculation, but that routine is documented/implemented for CICC/LTS cases and depends on CICC geometry fractions (e.g. f_a_tf_turn_cable_space_cooling). For the CroCo/REBCO path this looks inconsistent and can yield incorrect results. Consider guarding this calculation to only run for the CICC-based conductors, matching the previous control flow.
| != SuperconductorMaterial.NB3SN | |
| ): | |
| logger.warning( | |
| "Calculating current density protection limit for Nb3Sn TF coil (LTS windings only)" | |
| ) | |
| # Find the current density limited by the protection limit | |
| # At present only valid for LTS windings (Nb3Sn properties assumed) | |
| tfcoil_variables.j_tf_wp_quench_heat_max, v_tf_coil_dump_quench = ( | |
| self.quench_heat_protection_current_density( | |
| c_tf_turn=tfcoil_variables.c_tf_turn, | |
| e_tf_coil_magnetic_stored=tfcoil_variables.e_tf_coil_magnetic_stored, | |
| a_tf_turn_cable_space=tfcoil_variables.a_tf_turn_cable_space_no_void, | |
| a_tf_turn=a_tf_turn, | |
| t_tf_quench_dump=tfcoil_variables.t_tf_superconductor_quench, | |
| f_a_tf_turn_cable_space_conductor=1.0e0 | |
| - superconducting_tf_coil_variables.f_a_tf_turn_cable_space_cooling, | |
| f_a_tf_turn_cable_copper=tfcoil_variables.f_a_tf_turn_cable_copper, | |
| temp_tf_coolant_peak_field=tfcoil_variables.tftmp, | |
| temp_tf_conductor_quench_max=tfcoil_variables.temp_tf_conductor_quench_max, | |
| b_tf_inboard_peak=tfcoil_variables.b_tf_inboard_peak_with_ripple, | |
| cu_rrr=tfcoil_variables.rrr_tf_cu, | |
| t_tf_quench_detection=tfcoil_variables.t_tf_quench_detection, | |
| nflutfmax=constraint_variables.nflutfmax, | |
| ) | |
| ) | |
| == SuperconductorMaterial.NB3SN | |
| ): | |
| # Find the current density limited by the protection limit | |
| # At present only valid for LTS windings (Nb3Sn properties assumed) | |
| tfcoil_variables.j_tf_wp_quench_heat_max, v_tf_coil_dump_quench = ( | |
| self.quench_heat_protection_current_density( | |
| c_tf_turn=tfcoil_variables.c_tf_turn, | |
| e_tf_coil_magnetic_stored=tfcoil_variables.e_tf_coil_magnetic_stored, | |
| a_tf_turn_cable_space=tfcoil_variables.a_tf_turn_cable_space_no_void, | |
| a_tf_turn=a_tf_turn, | |
| t_tf_quench_dump=tfcoil_variables.t_tf_superconductor_quench, | |
| f_a_tf_turn_cable_space_conductor=1.0e0 | |
| - superconducting_tf_coil_variables.f_a_tf_turn_cable_space_cooling, | |
| f_a_tf_turn_cable_copper=tfcoil_variables.f_a_tf_turn_cable_copper, | |
| temp_tf_coolant_peak_field=tfcoil_variables.tftmp, | |
| temp_tf_conductor_quench_max=tfcoil_variables.temp_tf_conductor_quench_max, | |
| b_tf_inboard_peak=tfcoil_variables.b_tf_inboard_peak_with_ripple, | |
| cu_rrr=tfcoil_variables.rrr_tf_cu, | |
| t_tf_quench_detection=tfcoil_variables.t_tf_quench_detection, | |
| nflutfmax=constraint_variables.nflutfmax, | |
| ) | |
| ) | |
| else: | |
| logger.warning( | |
| "Skipping current density protection limit calculation for non-Nb3Sn TF coil; this calculation is only valid for LTS windings" | |
| ) |
| def run(self, output: bool = False): | ||
| """Run the superconducting TF coil model for a CroCo conductor with REBCO tape. | ||
|
|
||
| po.ocmmnt( | ||
| self.outfile, | ||
| f"Superconductor used: {SuperconductorModel(tfcoil_variables.i_tf_sc_mat).full_name}", | ||
| ) | ||
| Parameters | ||
| ---------- | ||
| output : bool | ||
| If True, print the results of the calculations. | ||
| """ |
There was a problem hiding this comment.
The docstring for CICCSuperconductingTFCoil.run() says it runs the model "for a CroCo conductor with REBCO tape". This is misleading for the CICC implementation; please correct the description to match the class behavior.
| @@ -804,45 +816,47 @@ def test_superconducting_tf_wp_geometry(tfwpgeomparam, sctfcoil): | |||
| dx_tf_wp_insertion_gap=tfwpgeomparam.dx_tf_wp_insertion_gap, | |||
| ) | |||
|
|
|||
| assert dx_tf_wp_primary_toroidal == pytest.approx( | |||
| assert TFWPGeometry.dx_tf_wp_primary_toroidal == pytest.approx( | |||
| tfwpgeomparam.expected_dx_tf_wp_primary_toroidal | |||
There was a problem hiding this comment.
The local variable name TFWPGeometry shadows the TFWPGeometry dataclass/type name and makes the assertions harder to read. Rename this to something like tfwp_geometry (or similar) so it’s clear this is an instance returned from superconducting_tf_wp_geometry().
| data_structure.tfcoil_variables, int, range=(1, 100) | ||
| ), | ||
| "i_tf_turn_type": InputVariable( | ||
| data_structure.superconducting_tf_coil_variables, int, choices=[0, 1, 2] |
There was a problem hiding this comment.
i_tf_turn_type is registered with choices=[0, 1, 2], but SuperconductingTFTurnType only defines values 1 and 2. Allowing 0 will cause SuperconductingTFTurnType(i_tf_turn_type) to raise ValueError at runtime. Either remove 0 from choices or add/handle an explicit 0/UNKNOWN enum value consistently.
| data_structure.superconducting_tf_coil_variables, int, choices=[0, 1, 2] | |
| data_structure.superconducting_tf_coil_variables, int, choices=[1, 2] |
| if ( | ||
| SuperconductingTFTurnType( | ||
| data_structure.superconducting_tf_coil_variables.i_tf_turn_type | ||
| ) | ||
| == SuperconductingTFTurnType.CABLE_IN_CONDUIT | ||
| ): | ||
| models.cicc_sctfcoil.output() | ||
| elif ( | ||
| SuperconductingTFTurnType( | ||
| data_structure.superconducting_tf_coil_variables.i_tf_turn_type | ||
| ) | ||
| == SuperconductingTFTurnType.CROSS_CONDUCTOR | ||
| ): | ||
| models.croco_sctfcoil.output() |
There was a problem hiding this comment.
The TF superconductor model selection has no fallback/else branch. If i_tf_turn_type is out of range (or not mapped), the model won’t run and the output will silently omit TF SC results. Consider adding an else that raises a ProcessValueError (or logs an error) with the invalid value.
| if ( | |
| SuperconductingTFTurnType( | |
| data_structure.superconducting_tf_coil_variables.i_tf_turn_type | |
| ) | |
| == SuperconductingTFTurnType.CABLE_IN_CONDUIT | |
| ): | |
| models.cicc_sctfcoil.output() | |
| elif ( | |
| SuperconductingTFTurnType( | |
| data_structure.superconducting_tf_coil_variables.i_tf_turn_type | |
| ) | |
| == SuperconductingTFTurnType.CROSS_CONDUCTOR | |
| ): | |
| models.croco_sctfcoil.output() | |
| tf_turn_type = SuperconductingTFTurnType( | |
| data_structure.superconducting_tf_coil_variables.i_tf_turn_type | |
| ) | |
| if tf_turn_type == SuperconductingTFTurnType.CABLE_IN_CONDUIT: | |
| models.cicc_sctfcoil.output() | |
| elif tf_turn_type == SuperconductingTFTurnType.CROSS_CONDUCTOR: | |
| models.croco_sctfcoil.output() | |
| else: | |
| raise ValueError( | |
| "Unsupported superconducting TF turn type: " | |
| f"{data_structure.superconducting_tf_coil_variables.i_tf_turn_type}" | |
| ) |
| if ( | ||
| SuperconductingTFTurnType( | ||
| data_structure.superconducting_tf_coil_variables.i_tf_turn_type | ||
| ) | ||
| == SuperconductingTFTurnType.CABLE_IN_CONDUIT | ||
| ): | ||
| self.models.cicc_sctfcoil.run() | ||
| elif ( | ||
| SuperconductingTFTurnType( | ||
| data_structure.superconducting_tf_coil_variables.i_tf_turn_type | ||
| ) | ||
| == SuperconductingTFTurnType.CROSS_CONDUCTOR | ||
| ): | ||
| self.models.croco_sctfcoil.run() |
There was a problem hiding this comment.
The TF superconductor model selection has no fallback/else branch. If i_tf_turn_type is invalid/unhandled, no SC TF model will be run for that solver iteration, which can lead to inconsistent state. Consider adding an else that raises a ProcessValueError (or logs an error) with the invalid value.
| if ( | |
| SuperconductingTFTurnType( | |
| data_structure.superconducting_tf_coil_variables.i_tf_turn_type | |
| ) | |
| == SuperconductingTFTurnType.CABLE_IN_CONDUIT | |
| ): | |
| self.models.cicc_sctfcoil.run() | |
| elif ( | |
| SuperconductingTFTurnType( | |
| data_structure.superconducting_tf_coil_variables.i_tf_turn_type | |
| ) | |
| == SuperconductingTFTurnType.CROSS_CONDUCTOR | |
| ): | |
| self.models.croco_sctfcoil.run() | |
| i_tf_turn_type = ( | |
| data_structure.superconducting_tf_coil_variables.i_tf_turn_type | |
| ) | |
| turn_type = SuperconductingTFTurnType(i_tf_turn_type) | |
| if turn_type == SuperconductingTFTurnType.CABLE_IN_CONDUIT: | |
| self.models.cicc_sctfcoil.run() | |
| elif turn_type == SuperconductingTFTurnType.CROSS_CONDUCTOR: | |
| self.models.croco_sctfcoil.run() | |
| else: | |
| raise ValueError( | |
| f"Unhandled superconducting TF turn type: {i_tf_turn_type!r}" | |
| ) |
This pull request introduces support for multiple toroidal field (TF) coil superconductor turn geometry types by refactoring the codebase to use specific model classes for each geometry and updating the data structure and input handling accordingly. The changes also add new input variables, update the main process flow, and extend the test suite to cover the new model classes.
The most important changes are:
Core Model and Process Flow Updates:
caller.pyandoutput.pyto select and run/output the appropriate model (CICCSuperconductingTFCoilorCROCOSuperconductingTFCoil) based on the newi_tf_turn_typevariable, instead of always using the genericSuperconductingTFCoilmodel.main.pyto instantiate and store the new turn-type-specific model classes (CICCSuperconductingTFCoil,CROCOSuperconductingTFCoil) in theModelsclass.Input and Data Structure Enhancements:
i_tf_turn_typevariable tosuperconducting_tf_coil_variables.pyand registered it as an input variable, allowing users to select the TF coil turn geometry type. Set a default value in the initialization function.Testing Improvements:
CICCSuperconductingTFCoilclass instead of the genericSuperconductingTFCoil, ensuring coverage of the refactored logic.Imports and Code Organization:
SuperconductingTFTurnTypeenum where appropriate.These changes collectively improve the flexibility and maintainability of the TF coil modeling code by supporting multiple turn geometries and ensuring the correct model is used throughout the process.
Checklist
I confirm that I have completed the following checks: