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

Adds utility for building the 'magnet surface' and shooting rays at it #142

Merged
merged 5 commits into from
Sep 7, 2024

Conversation

Edgar-21
Copy link
Contributor

No description provided.

@Edgar-21
Copy link
Contributor Author

I think #139 breaks this - but only a little. Once that gets merged I can update this

parastell/radial_distance_utils.py Outdated Show resolved Hide resolved
parastell/radial_distance_utils.py Outdated Show resolved Hide resolved
parastell/radial_distance_utils.py Outdated Show resolved Hide resolved
parastell/radial_distance_utils.py Outdated Show resolved Hide resolved
parastell/radial_distance_utils.py Outdated Show resolved Hide resolved
parastell/radial_distance_utils.py Outdated Show resolved Hide resolved
parastell/radial_distance_utils.py Outdated Show resolved Hide resolved
parastell/radial_distance_utils.py Outdated Show resolved Hide resolved
parastell/radial_distance_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one more place to use get_last_id - maybe...

parastell/radial_distance_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more changes...

Comment on lines 121 to 129
x1 = fil1[index, 0]
x2 = fil2[index, 0]
y1 = fil1[index, 1]
y2 = fil2[index, 1]
z1 = fil1[index, 2]
z2 = fil2[index, 2]
cubit.cmd(
f"create curve location {x1} {y1} {z1} location {x2} {y2} {z2}"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this?

Suggested change
x1 = fil1[index, 0]
x2 = fil2[index, 0]
y1 = fil1[index, 1]
y2 = fil2[index, 1]
z1 = fil1[index, 2]
z2 = fil2[index, 2]
cubit.cmd(
f"create curve location {x1} {y1} {z1} location {x2} {y2} {z2}"
)
loc1 = ' '.join(fil1[index,0:2])
loc2 = ' '.join(fil2[index,0:2])
cubit.cmd(
f"create curve location {loc1} location {loc2}"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to use list comprehension to make a list of str first but this will work. join() doesn't like to operate on iterables of things that are not str seems like

Comment on lines 37 to 39
if point[2] / next_point[2] < 0:
z_radius = calc_z_radius(point)
if max_z_radius < z_radius:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the max z point isn't where the filament crosses the x-y plane?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this is a poor variable name. It's called max_z_radius because its should end up being the point where it crosses the x-y plane with the larger distance to the z axis (has the larger radius if the center is the z axis) of the two crossings, not anything to do with the actual z value of the points. The if statement above checks for a sign change in z between the two points, and only checks for what the z radius is if there is a sign change (crosses the xy plane). I'll rename these to something hopefully a bit clearer.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this all together @Edgar-21

@gonuke gonuke merged commit f6549cb into main Sep 7, 2024
3 checks passed
@connoramoreno connoramoreno deleted the add_magnet_surface branch September 9, 2024 14:51
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