Skip to content

Refactor Gram-Schmidt #560

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jibril-b-coulibaly
Copy link
Contributor

@jibril-b-coulibaly jibril-b-coulibaly commented Mar 31, 2025

Refactor Gram-Schmidt to address concerns from #557.

  1. Fix bug in ChVector3.GetDirectionAxesAsY(). Luckily this function is never called... (7fb03c3)

  2. Create function ChVector3.GetDirectionAxes() that consolidates all 3 functions ChVector3.GetDirectionAxesAsX(), ChVector3.GetDirectionAxesAsY(), ChVector3.GetDirectionAxesAsZ() into one to avoid code duplication and perform verification tests to ensure the behavior is unchanged (1c85af8).

    • handle edge cases better by printing error and warning for invalid inputs (Please modify if needed, not sure how Chrono handles warning and errors so used std::cerr and std::runtime_error)

    • If suggested second vector is zero or collinear to first vector, modify it with a new heuristic that does not employ an unchecked while loop and guarantees normality, saving one Cross() calculation.

      • I used an int loop index instead of the char we discussed, the loop index will likely be put in a 32-bit register anyway and bring negligible memory savings so I thought it was more clear this way.

      • I took liberties to modify the heuristics considering that the correctness of a simulation must not depend on one particular choice of a rollback strategy when the input is invalid. If that werethe case, there is a bigger problem somewhere else with that simulation.

  3. Replace all calls to ChVector3.GetDirectionAxesAsX(), ChVector3.GetDirectionAxesAsY(), ChVector3.GetDirectionAxesAsZ() by call to ChVector3.GetDirectionAxes() with properly swapped arguments. Delete temporary verification test and extra comments (0bd5711)

  4. Other possible change to be discussed: testing for collinearity using the output of the Normalize() function is a bit extreme since this function checks for the minimum floating point value (minimum double is around 2.22507e-308). Many vectors that are near collinear can produce a cross product with a very small norm (e.g., 1e-15) that is still larger than the minimum double and Normalize() would not see this vector as being "near zero". This may lead to loss of precision and orthogonality. Using a less extreme test for how small is too small might be benefitial (see code comments in 1c85af8 for more)

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.

1 participant