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

Non-perpendicular hinge axis possible for multibody solver #299

Merged
merged 11 commits into from
Oct 30, 2024

Conversation

kccwing
Copy link
Collaborator

@kccwing kccwing commented Sep 10, 2024

Changes to hinge_for and hinge_node_for, and unit tests for multibody beam formulation using the augmented lagrangian method - cases with dominating scaling vs penalty factor checked, as well as a global rotation of the double pendulum with non-perpendicular axis. 3 new tests take in total of 3 minutes to run (3*30 time steps)

Second derivatives of constraint equations evaluated manually - to be replaced with AD version in due course.

(+ temporarily commented code for dynamic trim)
fixes to both constraint types, for augmented lagrange multiplier method
1. der_Tan_by_xv: limit for small crv
2. der_skewp_v and der_skewpT_v
if struct_tstep.mb_dict is None:
coords[counter, :] += struct_tstep.for_pos[0:3]
else:
#TODO: uncomment for dynamic trim
Copy link
Collaborator

@ben-l-p ben-l-p Sep 10, 2024

Choose a reason for hiding this comment

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

Does this need to be uncommented for dynamic trim, or can this be removed? (for this code and other similar comments in commit)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nope, these fix the issue that multibody simulations used to have outputs in the local A frame. The commented out portion will be useful for dynamic trim, but I haven't quite decided how best to proceed with it so I left the pointers there

sharpy/structure/utils/lagrangeconstraints.py Outdated Show resolved Hide resolved
Comment on lines 525 to 527
mat = ag.multiply_matrices(-np.eye(3))

vec = ag.multiply_matrices(-node_cga, node_dot_Ra)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to replace the multiply_matrices() method with a numpy one or @ as Ben mentioned? If the numpy method is done on an instance of a numpy array it should be fairly easy to replace with the find and replace. Otherwise, it might be more involved using sed?? (I'm not 100% sure how feasible it is)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am definitely a fan of the @ symbol for matrix multiplication - makes matrix algebra look much cleaner! Also, there's an identity matrix being multiplied by nothing here...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mat and vec structure is left intact for easier debugging and future work on these derivatives (until we replace the current approach) - open to replacing all with @ - will look into this and do it at least for the code involved in this PR.

@@ -1979,7 +1979,7 @@ def check(self):
raise RuntimeError(("'%s' parameter required in '%s' lagrange constraint" % (param, self.behaviour)))
has_behaviour = False
for param, value in self.__dict__.items():
if not param in ['behaviour', 'scalingFactor', 'penaltyFactor']:
if not param in ['behaviour', 'scalingFactor', 'penaltyFactor', 'rot_axisA2']:
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename rot_axisA2 to be consistent with the other params, e.g. rotAxisA2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

rot_axisA2 for backwards compatibility for existing job scripts, as well as consistency with rot_axisB in lagrangeconstraints.py

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah the joys of consistently inconsistent naming conventions 😂

Comment on lines 14 to 286
LC2.scalingFactor = 1e6
LC2.penaltyFactor = 0.

LC = []
LC.append(LC1)
LC.append(LC2)

MB1 = gc.BodyInformation()
MB1.body_number = 0
MB1.FoR_position = np.zeros((6,),)
MB1.FoR_velocity = np.zeros((6,),)
MB1.FoR_acceleration = np.zeros((6,),)
MB1.FoR_movement = 'free'
MB1.quat = ag.rotation2quat(ag.multiply_matrices(ag.rotation3d_z(lateral_ini),ag.quat2rotation(np.array([1.0,0.0,0.0,0.0]))))

MB2 = gc.BodyInformation()
MB2.body_number = 1
MB2.FoR_position = np.array([node_pos2[0, 0], node_pos2[0, 1], node_pos2[0, 2], 0.0, 0.0, 0.0])
MB2.FoR_velocity = np.zeros((6,),)
MB2.FoR_acceleration = np.zeros((6,),)
MB2.FoR_movement = 'free'
MB2.quat = ag.rotation2quat(ag.multiply_matrices(ag.rotation3d_z(lateral_ini),ag.quat2rotation(np.array([1.0,0.0,0.0,0.0]))))

MB = []
MB.append(MB1)
MB.append(MB2)

# Write files
gc.clean_test_files(SimInfo.solvers['SHARPy']['route'], SimInfo.solvers['SHARPy']['case'])
SimInfo.generate_solver_file()
SimInfo.generate_dyn_file(numtimesteps)
beam1.generate_h5_files(SimInfo.solvers['SHARPy']['route'], SimInfo.solvers['SHARPy']['case'])
gc.generate_multibody_file(LC, MB,SimInfo.solvers['SHARPy']['route'], SimInfo.solvers['SHARPy']['case'])

# sharpy_output = sharpy.sharpy_main.main(['',
# SimInfo.solvers['SHARPy']['route'] +
# SimInfo.solvers['SHARPy']['case'] +
# '.sharpy'])

# Same case with penalty weights
global name_hinge_slanted_pen
name_hinge_slanted_pen = 'name_hinge_slanted_pen'
SimInfo.solvers['SHARPy']['case'] = name_hinge_slanted_pen

LC1.scalingFactor = 1e-24
LC1.penaltyFactor = 1e0
LC2.scalingFactor = 1e-24
LC2.penaltyFactor = 1e0

gc.clean_test_files(SimInfo.solvers['SHARPy']['route'], SimInfo.solvers['SHARPy']['case'])
SimInfo.generate_solver_file()
SimInfo.generate_dyn_file(numtimesteps)
beam1.generate_h5_files(SimInfo.solvers['SHARPy']['route'], SimInfo.solvers['SHARPy']['case'])
gc.generate_multibody_file(LC, MB,SimInfo.solvers['SHARPy']['route'], SimInfo.solvers['SHARPy']['case'])

# Same case with rotated global reference
global name_hinge_slanted_lateralrot
name_hinge_slanted_lateralrot = 'name_hinge_slanted_lateralrot'
SimInfo.solvers['SHARPy']['case'] = name_hinge_slanted_lateralrot

LC2.rot_axisB = np.array([np.sin(45.0*deg2rad),np.cos(45.0*deg2rad),0.0])
LC2.rot_axisA2 = np.array([np.sin(45.0*deg2rad),np.cos(45.0*deg2rad),0.0])

gc.clean_test_files(SimInfo.solvers['SHARPy']['route'], SimInfo.solvers['SHARPy']['case'])
SimInfo.generate_solver_file()
SimInfo.generate_dyn_file(numtimesteps)
beam1.generate_h5_files(SimInfo.solvers['SHARPy']['route'], SimInfo.solvers['SHARPy']['case'])
gc.generate_multibody_file(LC, MB,SimInfo.solvers['SHARPy']['route'], SimInfo.solvers['SHARPy']['case'])

# # Same case with spherical joints
# global name_spherical
# name_spherical = 'dpg_spherical'
# SimInfo.solvers['SHARPy']['case'] = name_spherical

# SimInfo.solvers['NonLinearDynamicMultibody']['time_integrator'] = 'NewmarkBeta'
# SimInfo.solvers['NonLinearDynamicMultibody']['time_integrator_settings'] = {'newmark_damp': 0.15,
# 'dt': dt}

# LC1 = gc.LagrangeConstraint()
# LC1.behaviour = 'spherical_FoR'
# LC1.body_FoR = 0
# LC1.scalingFactor = 1e6

# LC2 = gc.LagrangeConstraint()
# LC2.behaviour = 'spherical_node_FoR'
# LC2.node_in_body = nnodes1-1
# LC2.body = 0
# LC2.body_FoR = 1
# LC2.scalingFactor = 1e6

gc.clean_test_files(SimInfo.solvers['SHARPy']['route'], SimInfo.solvers['SHARPy']['case'])
SimInfo.generate_solver_file()
SimInfo.generate_dyn_file(numtimesteps)
beam1.generate_h5_files(SimInfo.solvers['SHARPy']['route'], SimInfo.solvers['SHARPy']['case'])
gc.generate_multibody_file(LC, MB,SimInfo.solvers['SHARPy']['route'], SimInfo.solvers['SHARPy']['case'])

# sharpy_output = sharpy.sharpy_main.main(['',
# SimInfo.solvers['SHARPy']['route'] +
# SimInfo.solvers['SHARPy']['case'] +
# '.sharpy'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to break up the _set_up() method into smaller functions so that the method reads as a sequence of methods that explains the overall set-up is instead of needing to know all the individual steps?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, this is how the case generation class works as of current, which isn't part of this commit. It would maybe benefit from more documentation (I don't personally like using it as it is only convenient for simple cases), however as we already have a fair few cases defined using this, modifying it might not be a great idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is how they are all defined, is it worth having a base test class? Or are they sufficiently different?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are two methods of creating a model. There is the approach of writing every parameter separately (chord, elastic axis for each element node, structure connectivity etc etc) which can be a bit tedious at times, but that's what I always use. This class creates all this for you for simple configurations, for example a single cantilever wing without taper, or in this case a pendulum. Both will create the same H5 files which are passed to the solver. I personally feel that this case is fine - it preserves the structure of a "normal" case so hypothetically if you wanted to extend this case for a full simulation, for example a triple pendulum, then you very easily could. I don't see much benefit here in a special definition just for tests.

Copy link
Collaborator Author

@kccwing kccwing Sep 11, 2024

Choose a reason for hiding this comment

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

ideally i would create a class for the pendulum setup just like the flying wings for the model build, then another higher level of abstraction for case setup like what @ACea15 had done, but yes I do see value in this, especially the latter - I should have a good version of it by the time I am back in February. Thanks!

import shutil
from sharpy.utils.constants import deg2rad

class TestDoublePendulumSlanted(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is how we do unit tests in aerospace. But, this is not a unit test by the normal definition, as it should use mocking for the other components. This is technically an integration test. I'm ngl, I still haven't figured out what's an effective method for testing scientific code is 🤷‍♀️

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is how other tests are defined, so even if there is a better way to do this, I would rather they be consistent for now. We can look into swapping them all to a better definition in a future release.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I don't think we should change it, and I'm not 100% certain it would be worth it if we did????

Have you come across the hypothesis testing package? It's something I've briefly looked into and seems like it might be useful

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

indeed @wong-hl - I came across this when I was looking for a way to have a parametrised setup() within uniitest - haven't heard about mocking so I'll read up on it!

kccwing and others added 2 commits September 11, 2024 17:48
Mainly trimmed down lagrangeconstraints.py (numerics should be as before). Also cut some unneccesary comments I found along the way, and lastly cut the pesky multiply_matrices() function.
@ben-l-p ben-l-p self-requested a review October 9, 2024 15:43
Copy link
Collaborator

@ben-l-p ben-l-p left a comment

Choose a reason for hiding this comment

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

I cleaned up some of the code last week to try and make it a little bit less intimidating. I have been using this solver for a while already and have found it seemes to work as intended - again, great contribution Kelvin!

@kccwing
Copy link
Collaborator Author

kccwing commented Oct 9, 2024

Appreciate the help with the PR @ben-l-p and @wong-hl! Thank you again :)

Shall I merge this one as well and then it goes in the latest (minor) version?

@ben-l-p
Copy link
Collaborator

ben-l-p commented Oct 9, 2024

Appreciate the help with the PR @ben-l-p and @wong-hl! Thank you again :)

Shall I merge this one as well and then it goes in the latest (minor) version?

I'm happy with it - if Hui Ling is too then we should be good to go. I would say we should leave this in develop now and merge to main in due course, as it shouldn't take me too long (don't quote me on this) to add my multibody edits too, so it can all be in one version.

@kccwing
Copy link
Collaborator Author

kccwing commented Oct 9, 2024

Brilliant! No worries, and thanks again for the help and verification 😊

Copy link
Contributor

@wong-hl wong-hl left a comment

Choose a reason for hiding this comment

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

LGTM

@ben-l-p ben-l-p merged commit 69a4cdf into develop Oct 30, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants