Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEMO reactor #733

Merged
merged 32 commits into from
Mar 7, 2021
Merged

DEMO reactor #733

merged 32 commits into from
Mar 7, 2021

Conversation

RemDelaporteMathurin
Copy link

@RemDelaporteMathurin RemDelaporteMathurin commented Feb 17, 2021

Proposed changes

A tiny PR adding an example script. What do you think @shimwell ?

Types of changes

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

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code refactoring
  • Documentation Update (if none of the other choices apply)
  • New tests

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.

  • Pep8 applied
  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

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...

@codecov
Copy link

codecov bot commented Feb 17, 2021

Codecov Report

Merging #733 (c5b3336) into develop (8cfcded) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #733      +/-   ##
===========================================
+ Coverage    95.38%   95.42%   +0.03%     
===========================================
  Files           72       73       +1     
  Lines         4724     4762      +38     
===========================================
+ Hits          4506     4544      +38     
  Misses         218      218              
Impacted Files Coverage Δ
paramak/reactor.py 99.14% <ø> (ø)
paramak/__init__.py 100.00% <100.00%> (ø)
...aramak/parametric_reactors/eu_demo_2015_reactor.py 100.00% <100.00%> (ø)
paramak/shape.py 99.65% <100.00%> (+<0.01%) ⬆️

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 8cfcded...c5b3336. Read the comment docs.

@shimwell
Copy link
Collaborator

Thanks for making this Remi. I think it is just what we needed. Lets move it to a class, add some tests and some docs in a similar way to PR #734

@shimwell
Copy link
Collaborator

Current model looks like this

Screenshot from 2021-02-17 19-30-58

@shimwell
Copy link
Collaborator

I've moved the example in to a class object, added tests and added some documentation.

I think the general shapes need a bit more work (especially that overlap) and the documentation picture needs updating

@RemDelaporteMathurin
Copy link
Author

Perhaps the TF coils need an additionnal bit of work cause only the cases are described in this paper. Plus there's an overlap with the divertor

@shimwell
Copy link
Collaborator

Perhaps the TF coils need an additionnal bit of work cause only the cases are described in this paper. Plus there's an overlap with the divertor

I guess we can even not have coils in the model to begin with

@shimwell
Copy link
Collaborator

Would you like me to split this reactor up into sections like I've done for the Sparc one?

@RemDelaporteMathurin
Copy link
Author

Would you like me to split this reactor up into sections like I've done for the Sparc one?

I can try and have a go at it today. What about the coils though, should we remove them for now ?

@shimwell
Copy link
Collaborator

Would you like me to split this reactor up into sections like I've done for the Sparc one?

I can try and have a go at it today. What about the coils though, should we remove them for now ?

Yep lets remove the TF coils and just leave the TF cases. Thanks for this Remi, much appreachiated

@RemDelaporteMathurin
Copy link
Author

Would you like me to split this reactor up into sections like I've done for the Sparc one?

I can try and have a go at it today. What about the coils though, should we remove them for now ?

Yep lets remove the TF coils and just leave the TF cases. Thanks for this Remi, much appreachiated

But even the casing is overlapping I'm afraid. How about removing both the coils and casing until a better diagram is found ?

@shimwell
Copy link
Collaborator

Perhaps we can make the casing using an extrudeMixedShape and some coordinates

@shimwell
Copy link
Collaborator

@Derek-yfqiu this reactor model is based on a drawing in a published paper and has the curved magnet cases that you were looking for.

@RemDelaporteMathurin
Copy link
Author

@shimwell I've pushed some aditionnal shapes, but something doesn't work with the vacuum vessel... can't figure out what.

@shimwell
Copy link
Collaborator

I think the points might be messed up on the outer section halfway up
Screenshot from 2021-02-27 13-45-52

@RemDelaporteMathurin
Copy link
Author

I think the points might be messed up on the outer section halfway up

Thanks for spotting this, it now works properly:
image

@RemDelaporteMathurin
Copy link
Author

@shimwell perhaps removing the cuts in the vacuum vessel (blanket + divertor) would speed up the computation time ? but there will be some small overlaps

@shimwell
Copy link
Collaborator

shimwell commented Mar 2, 2021

@shimwell perhaps removing the cuts in the vacuum vessel (blanket + divertor) would speed up the computation time ? but there will be some small overlaps

lets avoid the overlaps for now.

I have a half implemented method of speeding up the graveyard construction. So I hope that will help. Happy to leave this PR for a few days while I tidy things up elsewhere and fix that graveyard problem. Then I shall be back.

@RemDelaporteMathurin
Copy link
Author

@shimwell I'm afraid there's something else going on here. When I run the tests locally, test_vessel_construction takes forever. Removing the line cut=vac_vessel_inner, # hollow shape makes it run instantaneously.

This is rather weird...

@RemDelaporteMathurin
Copy link
Author

RemDelaporteMathurin commented Mar 3, 2021

I've noticed that some points in the vacuum vessel weren't ordered correctly. I've fixed it.

However, it seems that when trying to do a 360 rotation angle, the following error occurs:

paramak/reactor.py:305: in export_stp
    entry.export_stp(
paramak/shape.py:811: in export_stp
    exporters.export(self.solid, str(path_filename), exportType='STEP')
paramak/shape.py:138: in solid
    self.create_solid()
paramak/parametric_shapes/extruded_mixed_shape.py:99: in create_solid
    solid = self.perform_boolean_operations(solid, wedge_cut=cutting_wedge)
paramak/shape.py:1137: in perform_boolean_operations
    solid = cut_solid(solid, self.cut)
paramak/utils.py:128: in cut_solid
    solid = solid.cut(cutting_solid.solid)
/opt/conda/lib/python3.8/site-packages/cadquery/cq.py:3069: in cut
    newS = newS.clean()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <cadquery.occ_impl.shapes.Compound object at 0x7f3f5d14aee0>

    def clean(self: T) -> T:
        """Experimental clean using ShapeUpgrade"""

        upgrader = ShapeUpgrade_UnifySameDomain(self.wrapped, True, True, True)
        upgrader.AllowInternalEdges(False)
>       upgrader.Build()
E       OCP.Standard.Standard_NullObject: BRep_Tool:: TopoDS_Vertex hasn't gp_Pnt

/opt/conda/lib/python3.8/site-packages/cadquery/occ_impl/shapes.py:360: Standard_NullObject

I fear this is somehow related to issue #445 because replacing the RotateSplineShape by RotateStraightShape solves the issue.

@shimwell
Copy link
Collaborator

shimwell commented Mar 3, 2021

Nice detective work there Remi. I am not sure what to do about the 360 rotation error, perhaps this is a question for CQ

@RemDelaporteMathurin
Copy link
Author

@shimwell I've opened #757 to try and solve this issue. A workaround for now would be to change the TF coils azimuth_placement_angles to avoid that they are placed at zero degree.

@RemDelaporteMathurin
Copy link
Author

@shimwell this is ready to be merged I think. However, the diff coverage is low. This is due to neutronics_utils.py, which is weird cause it's unchanged in this PR. Don't know what to do about it

@RemDelaporteMathurin
Copy link
Author

@shimwell @billingsley-john this is now ready for review! :-D

RemDelaporteMathurin and others added 3 commits March 6, 2021 22:27
Co-authored-by: Jonathan Shimwell <jonathan.shimwell@ukaea.uk>
Co-authored-by: Jonathan Shimwell <jonathan.shimwell@ukaea.uk>
Co-authored-by: Jonathan Shimwell <jonathan.shimwell@ukaea.uk>
@shimwell
Copy link
Collaborator

shimwell commented Mar 6, 2021

I think we just need a FreeCAD picture with a similar color scheme to the others, but I can do that when I build it over here

Co-authored-by: Jonathan Shimwell <jonathan.shimwell@ukaea.uk>
@shimwell
Copy link
Collaborator

shimwell commented Mar 6, 2021

We should also update the grid of 6 reactors that we have but could do with one more reactor to make a 3 by 3 grid

Screenshot from 2021-03-06 23-24-24

@RemDelaporteMathurin
Copy link
Author

We should also update the grid of 6 reactors that we have but could do with one more reactor to make a 3 by 3 grid

Real nice! Are you talking about the Figure 4 of the paper ? If so are you thinking about adding SPARC, DEMO and another one ?

We could try ITER...

@shimwell shimwell merged commit cf727de into develop Mar 7, 2021
@shimwell shimwell deleted the demo_reactor branch March 7, 2021 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants