Skip to content

🎨 Update the vertical build variables to new style#3523

Merged
timothy-nunn merged 17 commits intomainfrom
update_vertical_build_variables_for_clarity
Mar 11, 2025
Merged

🎨 Update the vertical build variables to new style#3523
timothy-nunn merged 17 commits intomainfrom
update_vertical_build_variables_for_clarity

Conversation

@chris-ashe
Copy link
Copy Markdown
Collaborator

@chris-ashe chris-ashe commented Feb 3, 2025

Namespace changes

Variables

  • thshield_vb -> dz_shld_thermal
  • vgap_xpoint_divertor -> dz_xpoint_divertor
  • divfix -> dz_divertor
  • vgap_vv_thermalshield -> dz_shld_vv_gap
  • vgaptop -> dz_fw_plasma_gap
  • blnktth -> dz_blkt_upper
  • fwtth -> dz_fw_upper
  • d_vv_bot -> dz_vv_lower
  • d_vv_top -> dz_vv_upper
  • shldtth -> dz_shld_upper
  • shldlth -> dz_shld_lower

Just cause it annoyed me and isnt part of the vertical build tf_in_cs has been changed.

  • tf_in_cs -> i_tf_inside_cs

✨ New additions

Two new X-point variables have been added to allow parsing of the plasma heights. The two new variables now allow separate heights for the upper and lower parts of the plasma. This should enable top-down asymmetry in the future if we decide to use those plasma shapes.

  • z_plasma_xpoint_upper: Represents the upper height of the plasma from the midplane to the upper X-point
  • z_plasma_xpoint_lower: Represents the lower height of the plasma from the midplane to the lower X-point

🐛 Bugs

The upper and lower plasma heights were originally set as rminor*kappa and this is how they were viewed in the OUT.DAT and MFILE.DAT files. This would cause a parsing error due to * so the variables could never be used. They also do not discern between the upper and lower plasma X-points.

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.

@chris-ashe chris-ashe self-assigned this Feb 3, 2025
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 3, 2025

Codecov Report

❌ Patch coverage is 6.89655% with 81 lines in your changes missing coverage. Please review.
✅ Project coverage is 31.20%. Comparing base (0ace3f9) to head (3c3ee34).
⚠️ Report is 464 commits behind head on main.

Files with missing lines Patch % Lines
process/build.py 0.00% 44 Missing ⚠️
process/geometry/firstwall_geometry.py 0.00% 5 Missing ⚠️
process/io/plot_proc.py 0.00% 5 Missing ⚠️
process/sctfcoil.py 0.00% 5 Missing ⚠️
process/geometry/blanket_geometry.py 0.00% 4 Missing ⚠️
process/init.py 0.00% 4 Missing ⚠️
process/geometry/shield_geometry.py 0.00% 3 Missing ⚠️
process/io/plot_radial_build.py 0.00% 3 Missing ⚠️
process/blanket_library.py 33.33% 2 Missing ⚠️
process/geometry/vacuum_vessel_geometry.py 0.00% 2 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3523      +/-   ##
==========================================
- Coverage   31.21%   31.20%   -0.01%     
==========================================
  Files          83       83              
  Lines       19938    19940       +2     
==========================================
  Hits         6223     6223              
- Misses      13715    13717       +2     

☔ 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 update_vertical_build_variables_for_clarity branch 2 times, most recently from 17442ef to 7a0220e Compare February 7, 2025 23:55
@chris-ashe chris-ashe force-pushed the update_vertical_build_variables_for_clarity branch 2 times, most recently from 54d88d8 to 7185be1 Compare February 20, 2025 08:57
@chris-ashe chris-ashe marked this pull request as ready for review February 20, 2025 14:56
Copy link
Copy Markdown
Collaborator

@timothy-nunn timothy-nunn 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 code. One comment re obsolete variables

Will need an additional reviewer to check new variables make sense, are necessary, and implemented correctly.

Comment thread process/io/obsolete_vars.py
@chris-ashe chris-ashe requested a review from j-a-foster March 6, 2025 11:05
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 the new variable names.

chris-ashe and others added 17 commits March 11, 2025 10:59
Co-authored-by: Timothy <75321887+timothy-nunn@users.noreply.github.com>
@chris-ashe chris-ashe force-pushed the update_vertical_build_variables_for_clarity branch from b9ee2c5 to 3c3ee34 Compare March 11, 2025 11:00
@timothy-nunn timothy-nunn merged commit 159ac62 into main Mar 11, 2025
@timothy-nunn timothy-nunn deleted the update_vertical_build_variables_for_clarity branch March 11, 2025 13:11
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.

5 participants