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

Allow zeroth order derivative for Bezier Curve #3596

Conversation

pepbos
Copy link
Contributor

@pepbos pepbos commented Nov 1, 2023

This PR changes calcValue to be a special case of calcDerivative with order zero.

This allows for less if / else switches further down the line and focuses calculations to a single function body.
Otherwise does not change the output of calcValue or calcDerivative.

Brief summary of changes

In SmoothSegmentedFunction

  • calcDerivative allows order zero.
  • calcValue calls calcDerivative with order zero.

Similarly, in SegmentedQuinticBezierToolkit:

  • calcQuinticBezierCurveDerivU and calcQuinticBezierCurveDYDX allow for order zero.
  • calcQuinticBezierCurveCurveVal calls calcQuinticBezierCurveDerivU with order zero.

Changed MustThrow unit test accordingly.

Testing I've completed

  • Unit tests are still running.
  • No significant change in performance (wall clock time) when simulating Rajagopal.
  • No change in simulation result of Rajagopal.

CHANGELOG.md

Change too minor?


This change is Reviewable

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

One minor comment, but otherwise looks good.

@pepbos I think one reviewer for this change is fine, max two.

"SegmentedQuinticBezierToolkit::calcQuinticBezierCurveDerivU",
"Error: order must be greater than.");
"Error: order must be greater than zero.");
Copy link
Member

Choose a reason for hiding this comment

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

Should be "...greater than or equal to zero."

@pepbos
Copy link
Contributor Author

pepbos commented Nov 2, 2023

@nickbianco thanks for reviewing :)

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

LGTM.

@pepbos pepbos merged commit 5ad501b into opensim-org:main Nov 6, 2023
6 checks passed
pepbos added a commit to ComputationalBiomechanicsLab/opensim-core that referenced this pull request Nov 7, 2023
pepbos added a commit that referenced this pull request Nov 10, 2023
…DomainBug

Fixes bug introduced by #3596 when handling NaNs.
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