Skip to content

Conversation

@billingsley-john
Copy link

@billingsley-john billingsley-john commented Dec 1, 2020

Proposed changes

PR which fixes the problem with port positioning. The z_pos parameter has been replaced with a center_point parameter which allows the port cutter faces to be translated in 2 dimensions rather than 1. This ensures placement can be controlled in all workplanes.

PR also attempts to add automatic port cutting to the parametric reactors.
A utility method called perform_port_cutting has been added to the utility.py script to perform this function.
CURRENT STATUS: Works as expected for large ports and small fillet radii.
KNOWN BUGS: For small ports, sometimes not all ports are cut from the outboard blankets and large fillet radii can cause errors.

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

No further comments

@billingsley-john billingsley-john linked an issue Dec 2, 2020 that may be closed by this pull request
@billingsley-john
Copy link
Author

UPDATE: With these changes made to the points attributes of these parametric components z_pos translates the port cutters in the expected direction.
However, perhaps z_pos is a misleading parameter name as direction this parameter translates the solid depends on the workplane specified by the user. I.e. it translates along the axis of the first letter of the workplane. Maybe something like 'vertical_offset' may be more appropriate as a general parameter.

@RemDelaporteMathurin
Copy link

UPDATE: With these changes made to the points attributes of these parametric components z_pos translates the port cutters in the expected direction.
However, perhaps z_pos is a misleading parameter name as direction this parameter translates the solid depends on the workplane specified by the user. I.e. it translates along the axis of the first letter of the workplane. Maybe something like 'vertical_offset' may be more appropriate as a general parameter.

The term "vertical" may also be a bit confusing since some planes are "horizontal". Tricky

@billingsley-john
Copy link
Author

UPDATE: With these changes made to the points attributes of these parametric components z_pos translates the port cutters in the expected direction.
However, perhaps z_pos is a misleading parameter name as direction this parameter translates the solid depends on the workplane specified by the user. I.e. it translates along the axis of the first letter of the workplane. Maybe something like 'vertical_offset' may be more appropriate as a general parameter.

The term "vertical" may also be a bit confusing since some planes are "horizontal". Tricky

Good point. Maybe 'port_offset' could be suitable? Then in the docstrings it can be made clear that this is the offset from the origin in the direction of the first coordinate of the workplane?

@RemDelaporteMathurin
Copy link

UPDATE: With these changes made to the points attributes of these parametric components z_pos translates the port cutters in the expected direction.
However, perhaps z_pos is a misleading parameter name as direction this parameter translates the solid depends on the workplane specified by the user. I.e. it translates along the axis of the first letter of the workplane. Maybe something like 'vertical_offset' may be more appropriate as a general parameter.

The term "vertical" may also be a bit confusing since some planes are "horizontal". Tricky

Good point. Maybe 'port_offset' could be suitable? Then in the docstrings it can be made clear that this is the offset from the origin in the direction of the first coordinate of the workplane?

I wonder if the solution would be to make these ports more generic and ask the user to provide a (float, float) point coordinates in the workplane instead of setting (0, float). This would solve the issue of naming cause we could name this center_point for instance.

@billingsley-john
Copy link
Author

UPDATE: With these changes made to the points attributes of these parametric components z_pos translates the port cutters in the expected direction.
However, perhaps z_pos is a misleading parameter name as direction this parameter translates the solid depends on the workplane specified by the user. I.e. it translates along the axis of the first letter of the workplane. Maybe something like 'vertical_offset' may be more appropriate as a general parameter.

The term "vertical" may also be a bit confusing since some planes are "horizontal". Tricky

Good point. Maybe 'port_offset' could be suitable? Then in the docstrings it can be made clear that this is the offset from the origin in the direction of the first coordinate of the workplane?

I wonder if the solution would be to make these ports more generic and ask the user to provide a (float, float) point coordinates in the workplane instead of setting (0, float). This would solve the issue of naming cause we could name this center_point for instance.

I like the sound of this solution, it could solve all of these issues. I shall work on implementation.

@billingsley-john
Copy link
Author

z_pos parameter has been replaced by a center_point attribute to allow center of cutter face to be specified in two dimensions

@billingsley-john
Copy link
Author

number_of_coils is difficult to implement correctly alongside port_azimuth_placement_angle as one impacts the other. Therefore, number_of_coils will be removed as a parameter (it may be added back later)

@shimwell
Copy link
Collaborator

shimwell commented Dec 5, 2020

Quite a lot going on here. Wondering if the ports should be in an inherited version of the base ball and submersion reactors as not everyone will want ports. Would that make it any easier to implement?

@billingsley-john
Copy link
Author

Features implemented in #714 and #717

@billingsley-john billingsley-john deleted the fix_z_pos branch February 24, 2021 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

4 participants