Skip to content

Conversation

@harryswift01
Copy link
Contributor

Summary

This PR addresses issues related to the weighted torque calculation by reverting to the original method, optimizing the calculation to prevent crashes, and improving documentation for clarity.

Changes

Revert weighted torque calculation and improve documentation:

  • Restored the original method for computing weighted torque.
  • Clarified handling of small moment of inertia values in the docstring, explaining why values below machine precision are treated as zero.
  • Also included a ValueError check guard on the square root calculation to make sure that the square root does not have any negative numbers within it.
  • No functional changes beyond reverting to the previous calculation method.

Optimize weighted torque calculation to reduce crashes:

  • Added a check to skip the calculation if the torque is already zero, as highlighted in issue Cannot Compute Weighted Torque #55, reducing unnecessary calculations.
  • Retained the divide-by-zero check to ensure numerical stability when computing weighted torques.
  • Improved docstring to reflect the updated logic.

Impact

  • The reverted method restores the original weighted torque calculation, which maintains the standards that were followed in previous versions of the code but with an explanation as to why this is being calculated this way.
  • The optimization prevents unnecessary calculations when the torque is zero, potentially improving performance and preventing crashes in certain scenarios.
  • The improved documentation provides better clarity on the treatment of small moment of inertia values and the updated logic for weighted torque calculations.

- Restored the original method for computing weighted torque
- Clarified handling of small moment of inertia values in the docstring, explaining why values below machine precision are treated as zero
- No functional changes beyond reverting to the previous calculation method
- Added a check to skip the calculation if the torque is already zero, highlighted in issue #55, reducing unnecessary calculations
- Retained the divide-by-zero check to ensure numerical stability when computing weighted torques
- Improved docstring to reflect the updated logic
@harryswift01 harryswift01 added the bug Something isn't working label Mar 12, 2025
@harryswift01 harryswift01 requested review from jimboid and skfegan March 12, 2025 16:53
@harryswift01 harryswift01 self-assigned this Mar 12, 2025
@harryswift01 harryswift01 linked an issue Mar 12, 2025 that may be closed by this pull request
@skfegan
Copy link
Member

skfegan commented Mar 13, 2025

In the get_weighted_torques function (in GeometricFunctions.py), there is a check for if the moment of inertia is zero (line 359). I think that if the torques is not zero and the moment of inertia is zero, it should raise a divide by zero error and not just ignore the moment of inertia weighting while carrying on as if nothing unusual happened. As far as I can tell, the physics of the torques and moment of inertia mean that this case should not happen, but I think it is better to have error handling than just trusting it isn't an issue.

… of inertia is zero instead of proceeding with unweighted torque calculation.
@harryswift01 harryswift01 added this to the WP7 - Refactor milestone Mar 13, 2025
@harryswift01 harryswift01 changed the title 55 weighted torque bug Weighted Torque Bug Mar 13, 2025
Copy link
Member

@skfegan skfegan left a comment

Choose a reason for hiding this comment

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

All the logic I expected is there with appropriate error messages.

Copy link
Member

@jimboid jimboid left a comment

Choose a reason for hiding this comment

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

The code looks to me like it reflects the output of the discussion with Richard this morning. We basically have one scenario that the program can carry on by setting the torque to zero and two situations that would lead to a graceful crash with informative error messages.

@skfegan I'm not going to hit approve on this review, can you check that you agree and then approve in your review for Harry to merge.

@jimboid
Copy link
Member

jimboid commented Mar 13, 2025

@harryswift01 oddly this approved with a review ended in "comment"! Can you hold off merging until Sarah has looked too.

@jimboid
Copy link
Member

jimboid commented Mar 13, 2025

@harryswift01 oddly this approved with a review ended in "comment"! Can you hold off merging until Sarah has looked too.

Ah no Sarah was just fast!

@harryswift01 harryswift01 merged commit 1c1dd5a into main Mar 13, 2025
5 checks passed
@harryswift01 harryswift01 deleted the 55-weighted-torque-bug branch March 13, 2025 13:29
@jimboid jimboid modified the milestones: WP7 - Refactor, 1.0.0 release Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot Compute Weighted Torque

4 participants