ukaea / paramak

Create parametric 3D fusion reactor CAD models
https://paramak.readthedocs.io/en/main/
37 stars 12 forks source link

changed order of rotate_solid and compound creation #781

Closed billingsley-john closed 3 years ago

billingsley-john commented 3 years ago

Proposed changes

PR to fix tf coil bug outlined in #780. Thanks @katie-taylor for raising the issue. Previously, the outer tf coil and inboard leg were created separately and then made into a single compound before being passed to rotate_solid which placed a shape at each azimuth_placement_angle and unioned. CadQuery didn't like the compound being passed to rotate_solid. Now, the outer tf coil and inboard leg are passed to rotate_solid separately to place at each azimuthal placement angle and a compound of their shapes created after.

Types of changes

What types of changes does your code introduce to the Paramak? Put an x in the boxes that apply

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

billingsley-john commented 3 years ago

Inner and outer shapes now cut independently by cutting wedge before being combined in compound

codecov[bot] commented 3 years ago

Codecov Report

Merging #781 (b6293e3) into develop (a6c10ab) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #781   +/-   ##
========================================
  Coverage    95.25%   95.25%           
========================================
  Files           73       73           
  Lines         4780     4782    +2     
========================================
+ Hits          4553     4555    +2     
  Misses         227      227           
Impacted Files Coverage Δ
...metric_components/toroidal_field_coil_rectangle.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a6c10ab...b6293e3. Read the comment docs.

billingsley-john commented 3 years ago

@billingsley-john Great fix! Would it be possible to add a test to TFCoilRectangle() that would've caught that bug ? and perhaps to the other shapes that have a inner_leg=True flag ?

Yes, tests will be added and fixes to the other tf coils with the same issue will be made in a future PR by @katie-taylor

billingsley-john commented 3 years ago

@RemDelaporteMathurin - Test to catch this error has been added. Similar tests will be added to other tf coil components when fixing those.