Skip to content

Allow torques only#46

Merged
RaulPPelaez merged 30 commits intomainfrom
allow_torques_only
Jun 9, 2025
Merged

Allow torques only#46
RaulPPelaez merged 30 commits intomainfrom
allow_torques_only

Conversation

@rykerfish
Copy link
Contributor

@rykerfish rykerfish commented May 2, 2025

The goal here is to allow NBody and DPStokes to run without crashing when only torques are given.

DEPENDS ON #43

@rykerfish
Copy link
Contributor Author

I think this is fully functional for NBody. I templated the dotProducts so that it only computes the blocks it needs to for each configuration. This let us skip computing blocks that will always be zero in a seemingly flexible manner. Feel free to review the NBody part of this branch, but I'll still need to attempt to make DPStokes work with only torques before this is ready to go.

@RaulPPelaez
Copy link
Contributor

This is a rough merge with main I am afraid... There are some conflicts.

@rykerfish rykerfish force-pushed the allow_torques_only branch from 7d3e632 to b4fc93b Compare May 7, 2025 19:40
@rykerfish
Copy link
Contributor Author

This is a rough merge with main I am afraid... There are some conflicts.

Yeah... this does seem to indicate a bad workflow on my part. I think I messed up by rebasing instead of merging main into this branch, but it is standard to have to force-push to the feature branch after doing a rebase. The commit history from this branch is unfortunately pretty messed up. I vote to just squash-merge it into main once it's done and I'll pay attention to make sure we didn't lose any tests/features along the way.

Since we've been squash-merging all of our feature branches into main, I think in the future it makes sense to always merge main into the feature branches and resolve any conflicts there. I don't know why I tried a rebase this time, but I found it annoying to have to replay all commits from the branch on top of main.

@rykerfish
Copy link
Contributor Author

rykerfish commented May 7, 2025

@RaulPPelaez I think we should rename the needsTorques flag to something like includeAngular and change the behavior of the interface. I think this would be more consistent with the math when viewed from the perspective of what terms to include from the multiple expansion.

The motivation is that this PR now allows the user to input only one of forces or torques (i.e. not require forces when using torques like it used to) when needsTorques=True. Currently, inputting only forces or only torques will only return linear or angular velocities, respectively, even though a force can induce a angular displacement and vice versa. This can be circumvented by passing in forces and torques but with all torques set to zero, but this seems unintuitive to have to pass in torques to enable angular displacements under the hood. It used to make more sense when forces were always required when inputting torques.

Here's my proposal:
With includeAngular=False, only allow forces as input and only return linear velocities. With includeAngular=True, allow the user to input only forces, only torques, or both, but return both linear and angular velocities for all combinations. Maybe it would be possible for us to only return one variable with includeAngular=False but return two variables with includeAngular=True, but maybe that would be confusing.

@brennansprinkle do you have any thoughts? I think I'm just trying to knock down some assumptions we made in the old implementation for how Mdot would be used.

EDIT: this is also from the perspective of rewriting the structure of NBody. The above ideas may not work with DPStokes, so let me know if you think that's the case.

EDIT 2: I think this perspective works fine with DPStokes. When we want angular velocities, we probably just always run it with both torques and forces with decide what to throw away in the interface.

…x since forces are just filled with zeros under the hood.
@rykerfish
Copy link
Contributor Author

The fix for DPStokes was comparatively simple. It has the same problem that NBody does (e.g. if you only give torques you only get angular velocities) so my proposal above stays the same and applies to both solvers.

@rykerfish rykerfish marked this pull request as ready for review May 8, 2025 18:45
@rykerfish rykerfish requested a review from RaulPPelaez May 8, 2025 19:30
@RaulPPelaez
Copy link
Contributor

I agree with the includeAngular

@rykerfish
Copy link
Contributor Author

I think this implements an easier-to-maintain version of the NBody kernels and changes the interface to use includeAngular instead of needsTorque. Sorry about the delaying on finishing it!!

@rykerfish rykerfish requested a review from RaulPPelaez June 6, 2025 01:59
Copy link
Contributor

@RaulPPelaez RaulPPelaez left a comment

Choose a reason for hiding this comment

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

Its great, Ryker. Only thing left is to add the change to the docs

@RaulPPelaez
Copy link
Contributor

The only place in the docs where the change is needed is in the Usage example. Which makes me think that that code should be picked automatically from the examples folder. I am sure that is straight forward, but idk how to do it -.-
If you want just change the example for the moment and feel free to merge.
Thank you so much @rykerfish !

@rykerfish
Copy link
Contributor Author

@RaulPPelaez I was going to merge this but got caught up on whether a squash merge was correct for a PR of this size or not. Currently, I'm thinking that putting this many changes into a single squashed-commit could make our lives harder in the future for tracking down bugs/changes.

Thoughts on this? I'm leaning towards directly merging PRs like this with many changes across many files but continuing to squash-merge more minor changes that are less consequential. I'm a rebase-hater.

Let me know what you think and feel free to hit that merge button for whichever option you think is better!

@RaulPPelaez RaulPPelaez merged commit c20c8e2 into main Jun 9, 2025
2 checks passed
@RaulPPelaez
Copy link
Contributor

I like squash on small PRs and merge on large ones if there are many changes that are "unrelated". I am not sure what the best practice s here, honestly.
Squash is also good because a single commit with all changes has a link to the PR it originates from with a discussion about the changes. I decide what is more worth it on a case-by-case basis.
In this case, I just got you out of your decision block by selecting the option you were already leaning towards, hehe.

@rykerfish rykerfish deleted the allow_torques_only branch June 25, 2025 16:05
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