Currently the parametric reactor classes take a few parameters for the PF coil sizes (pf_coil_radial_thicknesses and pf_coil_vertical_thicknesses)

Also some parameters that position the PF coils radially (pf_coil_to_tf_coil_radial_gap and pf_coil_to_rear_blanket_radial_gap)

The vertical spacing is just shared equally between the number of PF coils.

While the existing setup is is great as it makes it difficult for users to make overlapping geometry we don't have quite enough flexibility to make PF coils in the correct positions.

Therefore I am proposing that ....

The two arguments for (pf_coil_to_tf_coil_radial_gap and pf_coil_to_rear_blanket_radial_gap) can be combined into one argument called rear_blanket_to_tf_gap

Another argument should be provided called pf_coil_vertical_position that can be used to control the y position of the PF coils (center points). This should be a list of numbers then the PF coils can each take a value

Another argument should be provided called pf_coil_radial_position that can be used to control the x position of the PF coils (center points). This should be a list of numbers then the PF coils can each take a value

While all of the radial thickness numbers sequentially build on each other the PF coils will be unique as the user provides a radial position so the user can therefore provide coordinates that overlap with existing geometry. It is therefore necessary to cut everything else with the PF coils to avoid overlaps, and also to check the user has not overlapped PF coils with themselves.

One downside is that this makes it harder to make reactor geometry but on the plus side it adds the flexibility needed

Comments, suggestions, feedback on this idea are most welcome,

Some test docstrings do not give a correct description of the test

Some parametric components still have last point=first point in their find_points() methods. These can all be removed.

The images for parametric components, shapes and reactors are nice but they don't show how the parameters change the shape of the models

One option is to make svg images using the exportsvg method then convert this to a png then add annotations to this image using latex

I had a quick go at this to see how it looks, perhaps this or something like this could be something we consider adding to the documentation in the future.

Currently the workplane has to be set to 'XY' as the exportsvg has no camera position argument.

The viewing angle of the SVG export in CADquery has currently hard coded viewing angle which is not perfect for the Paramak as it is from above.

Here is the default view

If you open up the exporter.py file in you source code for your local install of CADquery mine is saved in '/home/jshim/anaconda3/lib/python3.7/site-packages/cadquery/occ_impl/exporters.py or sometimes in /home/jshim/cadquery/cadquery/occ_impl/exporters.py' then you can change the default hard coded angle

From this
DEFAULT_DIR = gp_Dir(-1.75, 1.1, 5)
To something like this
DEFAULT_DIR = gp_Dir(-2, 5, 1.1)

The result is an improved view angle for the SVG export :-)

This is a a temporary fix as CADQuery will probably support viewing angle as an argument in export_svg in the future and there is an open issue for this CadQuery/cadquery#246

Including the lic header on each file is breaking the user documentation as shown in screen shot below.

Additionally there is a large maintenance burden to keeping the file lic header the same across every file

Additionally this lic header does not appear to be standard practice looking at other git repositories. The standard practice is to include a LICENSE.md or LICENSE in the root of the repository. This then allows github to automatically detect the license type.

the parametric shape doc strings state that the first and last points must be the same but this is no longer correct. Thanks to a user for reporting this

We have a read the docs for the develop branch call latest but nothing for the stable version

Perhaps we should introduce another readthedocs version

Wrong geometry

How to reproduce:

from paramak import BlanketConstantThicknessFP

my_shape = BlanketConstantThicknessFP(100, start_angle=0, stop_angle=360, rotation_angle=360)
my_shape.solid

Crash when calling export_stp() method with full coverage

How to reproduce:

from paramak import BlanketConstantThicknessFP

my_shape = BlanketConstantThicknessFP(100, start_angle=0, stop_angle=360, rotation_angle=360)
my_shape.export_stp('blanket.stp')

Enhancement: monving from reStructured style to Google style docstrings.

• reStructured text is convenient but is really dense and hard to type and to read

• IDE such as VS code can generate docstrings which saves time and efforts. See here

• Google style docstrings can be supported by Sphinx using the Napoleon extension for Sphinx

The Plasma() class would be documented like this:

class Plasma(RotateSplineShape):
    """Creates a double null tokamak plasma shape that is controlled
       by 4 shaping parameters.


    Attributes:
        points ([type], optional): [description]. Defaults to None.
        name (str, optional): [description]. Defaults to 'plasma'.
        material_tag (str, optional): [description]. Defaults to 'DT_plasma'.
        workplane (str, optional): [description]. Defaults to "XZ".
        elongation (float, optional): [description]. Defaults to 2.0.
        major_radius (int, optional): [description]. Defaults to 450.
        minor_radius (int, optional): [description]. Defaults to 150.
        triangularity (float, optional): [description]. Defaults to 0.55.
        vertical_displacement (int, optional): [description]. Defaults to 0.
        num_points (int, optional): [description]. Defaults to 50.
        configuration (str, optional): [description]. Defaults to "non-null".
        x_point_shift (float, optional): [description]. Defaults to 0.1.
        solid ([type], optional): [description]. Defaults to None.
        stp_filename (str, optional): [description]. Defaults to "plasma.stp".
        color ([type], optional): [description]. Defaults to None.
        rotation_angle (int, optional): [description]. Defaults to 360.
        azimuth_placement_angle (int, optional): [description]. Defaults to 0.
        cut ([type], optional): [description]. Defaults to None.
        hash_value ([type], optional): [description]. Defaults to None.

    Raises:
        ValueError: [description]
        ValueError: [description]
        ValueError: [description]
        ValueError: [description]
    """

instead of the current:

class Plasma(RotateSplineShape):
    """Creates a double null tokamak plasma shape that is controlled
       by 4 shaping parameters.

    :param major_radius: the major radius of the plasma (cm)
    :type major_radius: float
    :param minor_radius: the minor radius of the plasma (cm)
    :type minor_radius: float
    :param triangularity: the triangularity of the plasma
    :type triangularity: float
    :param elongation: the elongation of the plasma
    :type elongation: float
    :param vertical_displacement: the vertical_displacement of the plasma (cm)
    :type vertical_displacement: float
    :param num_points: number of points to described the shape
    :type num_points: int
    :param configuration: plasma configuration
     ("non-null", "single-null", "double-null"). Defaults to "non-null")
    :type configuration: str
    :param x_point_shift: Shift parameters for locating the X points in [0, 1].
     Default to 0.1.
    :type x_point_shift: float

    :return: a shape object that has generic functionality with 4 attributes
       (outer_equatorial_point, inner_equatorial_point, high_point, low_point)
       as tuples of 2 floats
    :rtype: paramak shape object
    """

Note: some attributes might be removed from this documentation for simpler code since they belong to the inherited class (hash_value, cut, color, solid...)

when exporting html scatter graphs of the reactor or exporting 2d_images the methods rely on points in the object.

These points are not fully representative of the objects just to the interconnections sometimes being circles or splines

One example is when creating a blanket from an arc then the three points show up as a triangle

One solution for this is to get matplotlib to use the connections between points, another solution is to faceted the shapes using the method described here

https://groups.google.com/forum/#!searchin/cadquery/spline%7Csort:date/cadquery/Ia84dtcnCo0/CCvVIl1bBQAJ

I think @billingsley-john might be interested as this also causes problems for the sweep circle operations

We could also move to a completely different way of viewing the objects using a slice through the CAD model or make use of openmc plotting

I'm keen on the faceting idea as I think it could come in useful when trying to export splineless CAD

All of the blankets are Constant Thicknesses which is great but occasionally there is a need for a variable thickness blanket

For example the EU DEMO blanket has a thicker outboard blanket than the inboard blanket https://images.app.goo.gl/5FCPGWVKosSfgJ7Z7

perhaps thickness could allow a list of two thickness and then linear interpolation between the 2 points or a start_thickness and stop_thickness could be used instead

The docstrings for parametric shapes, components and plasmas do not document kwargs

Currently we have a few keyword arguments for shape.py

Adding new keywords such as the recently added tet_mesh and physical_groups to the keywords propagates to the super statements which means updating all the classes that inherit.

Therefore they have been added as attributes but not as arguments.

This is a shame as uses would have to may shapes then set attributes after the shape object is initialized. Users can't currently pass extra keyword on creation of the shape object which is a shame.

This can be fixed to allow users to add attributes or keyword args to the object on creation
https://treyhunner.com/2018/04/keyword-arguments-in-python/

I was thinking some auto pep 8 might be handy for example ...

https://github.com/marketplace/actions/autopep8

I'm sure there are other options

Would be great to have a parametric reactor that just has a single divertor

This could probably just be a small change to the existing BallReactor and SubmersionTokamak that takes an ['upper'],['lower'] or ['upper', 'lower'] argument and then makes the diverter envelop a little shorter. The divertor envelop is used in conjunction with the intersect feature and th firstwall geometry to make the divertor. The height of the divertor envelope can be reduced so that it only covers the upper or lower part of the firstwall

Y value of lower_x_point should be negative if vertical_displacement is zero.

There is an option to have an open source method of making h5m dagmc files.

Here is a demo

https://gist.github.com/pshriwise/52452c37d4b7dd89bdc9374e13c35157

If this were to be implemented we would need to ensure the STL geometry does not overlap on the curves. This could perhaps be done by adding lots of control points to the geometry

In #138 we added some more tests in order to improve the code coverage but some are failing. In particular the one involving export_3d_image method.

See this build

Circle Ci builds are not being triggered from forks. This could be because the forks are outside the UKAEA organisation

find_number_of_volumes_in_each_step_file() function cannot be split over lines, must be one complete line

CURRENT:

geometry_details = find_number_of_volumes_in_each_step_file(
[indent] geometry_details, str(os.path.abspath("."))
)

FIX TODO:

geometry_details = find_number_of_volumes_in_each_step_file(geometry_details, str(os.path.abspath(".")))

Currently, html graphs are plotted with straight lines between each point. Last point=first point is appended to self.points which facilitates straight line between [-1] point and last point but prevents the hovertext for first point being seen. At some point, it would be better to join the points by connection type instead of straight lines. Also worth considering whether last point=first point needs to be added to self.points?

The current install process is a bit messy with a mix of conda and pip

Lets make a conda install and make it easier for people to get started

There are a few how to guides e.g.
https://medium.com/@giswqs/building-a-conda-package-and-uploading-it-to-anaconda-cloud-6a3abd1c5c52

How to reproduce

# Doesn't work
test_shape = BlanketFP(
    major_radius=300,
    minor_radius=50,
    triangularity=0.5,
    elongation=2,
    thickness=200,
    stop_angle=360,
    start_angle=0,
)
assert test_shape.solid is not None
# Works
test_shape = BlanketFP(
    major_radius=300,
    minor_radius=50,
    triangularity=0.5,
    elongation=2,
    thickness=200,
    stop_angle=340,
    start_angle=0,
)
assert test_shape.solid is not None
test_shape = BlanketFP(
    major_radius=300,
    minor_radius=50,
    triangularity=0.5,
    elongation=2,
    thickness=200,
    stop_angle=360,
    start_angle=0,
    rotation_angle=180
)
assert test_shape.solid is not None

color specification doesn't seem to be working correctly for export_2d_image.
perhaps it would be good to add more details on how to specify the RGB/RGBA values for color in the documentation

It is not convention to add docstrings to property setters - docstrings are only usually added to a property under @Property. Sphinx ignores docstrings in property setters because of this. Some property setters have been documented but these docstrings do not appear in the documentation. This should be changed.

Thanks to the system of differential equations described here, Princeton-D TF coils can be added to the Paramak.

The pypi package is rebuild when a PR is made to the main branch. Subsequent changes to the PR (changes to develop branch) cause pypi package to rebuild, however, rebuild fails because we already have a build of the same version number. Therefore, subsequent changes to the PR are not incorporated into the pypi package.
Would it be better to only rebuild the pypi package once the PR has been merged? I.e. pypi package rebuild on commits to main, rather than PR's to main?

The current implementation of the BlanketConstantThicknessArcV and BlanketConstantThicknessArcH draw a circle from three points for the inner surface and with another circle for the rear surface.

The top and bottom points are made by either vertical (BlanketConstantThicknessArcV) or horizontal (BlanketConstantThicknessArcV) projection of the inner point by the amount specified in the thickness argument.

For the BlanketConstantThicknessArcV this approach is reasonable when a half circle is made as the points are perpendicular from the inner points. However in most cases this ends up with a thinner top part and bottom part compared to the middle. The distance for the vertical and horizontal point need calculating using the circle center point and some trigonometry to get it correct.

Here are some images that show when the current implementation is a problem and also when it is not

Here we can see that the upper and lower parts are thinner

Here we can see that the upper and lower parts are the correct thickness

hash_value is currently calculated individually for each parametric_shape by specifying each parameter individually. This needs to be changed so that the get_hash method calculates the hash_value by automatically detecting the class parameters

The code coverage of the test suite has increased

https://codecov.io/gh/ukaea/paramak/branch/develop

however it is still not at 90%

This is partly because some parts of shape.py are difficult to test in the docker image due to graphics problems.

The other parts are the divertor block that should be either tested or removed

If I understood correctly, the component plasma is created from 2 arcs.

Why not using the actual analytical expression for the plasma boundary ? (based on elongation, triangularity, minor and major radii)

This would give a smoother and perhaps more accurate shape ?

The X points should be functions of vertical_displacement attribute

Because both ITER divertor classes are grouped in the same script, the image for the no-dome ITER divertor is in the wrong place in the documentation

A low effort addition that will facilitate contribution (imho)

I have been testing the BlanketFC using a tuple of two thicknesses and appear to be getting incorrect results.

I was expecting the blanket thickness to vary (in linear manner) but it appears to be constant in one case and I'm really not sure what is happening in the other image but added it for fun

Code to reproduce :

import paramak

plasma = paramak.Plasma(minor_radius=200, major_radius=620, triangularity=0.55, elongation=1.85)
plasma.solid

When the argument rotation_angle is not given, the .solid method doesn't display the plasma.

To reproduce geometry relevant for EU DEMO designs the blankets need a little more flexibility.

In this case it looks like a variable offset from the plasma is needed to match the EU DEMO blanket shape

The segmentation of the blanket is looking better and an example has been added to the example_parametric_components branch. https://github.com/ukaea/paramak/blob/demo/examples/example_parametric_components/make_demo_style_blankets.py
Eventually this can be part of a demo model

STL files should be exportable from the reactor like STP files are.

This would allow future facet based ray tracing to be performed

The images which are currently hosted on google drive and are not loading up every time the Readme is loaded.

Remi has shown us another way using github issues to host images which we can try :-)

color scheme uses https://colorbrewer2.org/#type=qualitative&scheme=Set1&n=8

We could do with a more focused Readme.

I'm assuming GitHub is where developers and source code people will go

My concern is that they see the examples and don't go to the documentation

So I'm thinking we should move the examples over and or make the examples in the documentation better.

Any one have any thoughts on this

the home page has a few out of date things

Usage - Shape creation should include images and be renamed to something like Usage - parametric shape

Usage - Creating a Plasma should be renamed to something like this Usage - parametric shape creation

Import statement is wrong for the plasma

Usage - Reactor creation should include images and be changed to Usage - parametric reactor

the parametric reactor example uses add which no longer exists

utils.py should also be added to the "Core modules" section

Perhaps the long list of python classes could be removed from the index page

As described in #164, apply to submersion reactor

The make_plasmas.py example script has 4 basic plasma and one more advanced plasma.

Could this script be upgraded to plot the basic plasma shapes on te left by multiplying their x values by -1 and the advance plasma shapes on the right

3 more advanced plasma also be added to the script using these input parameters

@billingsley-john

Some of these attributes aren't actual attributes but rather the parameters. These should be corrected

The current parametric reactors (ball, submersion and single null) accept optional arguments which add in pf and outboard tf coils. Should the tf coil-related arguments have 'outboard' in their name to distinguish between the inboard and outboard tf coils?

I think this image is now building

https://github.com/Shimwell/paramak_dependencies/blob/master/Dockerfile_with_pymoab

We could update the CI to use this image or replace the current image with this one.

It should allow us to pass those export_h5m tests

BallReactor has intermittent construction error, either defined as StandardConstructionError or a 'NoneType' error. Error can be reproduced both locally and within the openmcworkshop/paramak_dependencies docker image

Parametric shapes don't currently have the same **kwarg based super as parametric components

This would allow keywords to be passed on construction and add some flexibility to the code

Like tet_mesh #170 , the attribute physical_groups can be added to the constructor of the shape class and added to the default_dict. This allows physical_groups to be specified upon construction (or default to None), as well as modified after, but 'hides' the parameter from the user.

What do you think @RemiTheWarrior ?