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

points are not reusable between Shapes #751

Open
shimwell opened this issue Feb 27, 2021 · 6 comments · Fixed by #839
Open

points are not reusable between Shapes #751

shimwell opened this issue Feb 27, 2021 · 6 comments · Fixed by #839

Comments

@shimwell
Copy link
Collaborator

Points is passed by the user and changed internally by the class (perhaps this is poor program design).

One change adds the start point as the end point to make a closed wire. This is nice as it saves the user time and having to make that last point which the program can easily figure out.

It also checks that the input first and last points are not the same.

Anyway the points is pass as a list which is mutable.

Therefore the list values are changed and can't be reused. This simple example produces an error.

Perhaps we should not change the points that the user inputs by make a copy and change the copy?

points =[
        (100, 0, "straight"),
        (200, 0, "straight"),
        (250, 50, "straight"),
        (200, 100, "straight"),
    ]

test_rotate_mixed_shape = paramak.RotateMixedShape(
    rotation_angle=180,
    points=points
)
test_extrude_mixed_shape = paramak.ExtrudeMixedShape(
    distance=10,
    points=points
)
ValueError                                Traceback (most recent call last)
<ipython-input-9-eeb600e00657> in <module>
     10     points=points
     11 )
---> 12 test_extrude_mixed_shape = paramak.ExtrudeMixedShape(
     13     distance=10,
     14     points=points

~/paramak/paramak/parametric_shapes/extruded_mixed_shape.py in __init__(self, distance, extrude_both, rotation_angle, extrusion_start_offset, stp_filename, stl_filename, **kwargs)
     33     ):
     34 
---> 35         super().__init__(
     36             stp_filename=stp_filename,
     37             stl_filename=stl_filename,

~/paramak/paramak/shape.py in __init__(self, points, connection_type, name, color, material_tag, stp_filename, stl_filename, azimuth_placement_angle, workplane, rotation_axis, tet_mesh, surface_reflectivity, physical_groups, cut, intersect, union)
     94 
     95         self.connection_type = connection_type
---> 96         self.points = points
     97         self.stp_filename = stp_filename
     98         self.stl_filename = stl_filename

~/paramak/paramak/shape.py in points(self, values)
    465                     msg = "The coordinates of the last and first points are \
    466                         the same."
--> 467                     raise ValueError(msg)
    468 
    469                 values.append(values[0])

ValueError: The coordinates of the last and first points are                         the same.
@RemDelaporteMathurin
Copy link

I noticed that a while ago, and it was a tad annoying 😸

@shimwell
Copy link
Collaborator Author

Another related problem is that if one wants to reuse the points in another shape then I think they will have been modified by the first shape and potentially unusable in the second shape

shape1 = paramak.RotateStraightShape(
    points= [
    (100, 0),
    (200, 0),
    (250, 50),
    (200, 100),
]
)

# shape1.points is now a list with connections not what the user originally input

shape1 = paramak.RotateSplineShape(
    points= shape1.points
]
)

@RemDelaporteMathurin
Copy link

One way to solve this would be to have two separate attributes points and connections.

One would only contain coordinates and the latter will only contain strings?

@shimwell
Copy link
Collaborator Author

Or perhaps another version of points called processed_points or something like that

@RemDelaporteMathurin RemDelaporteMathurin linked a pull request May 28, 2021 that will close this issue
10 tasks
@RemDelaporteMathurin
Copy link

@shimwell I think this can be closed since it was solved in #839

@shimwell
Copy link
Collaborator Author

Do you mind if I keep it open a bit longer, I need to get that example I posted a few comments up working

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 a pull request may close this issue.

2 participants