-
-
Notifications
You must be signed in to change notification settings - Fork 200
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
Fix derive for degree == 1 and update unit test #164
Conversation
The derive_discontinuous_lines_ignoring_epsilon unit test is currently failing.
Hi @justinthomas, thank you again for your efforts.
Will you fix it? |
slope[0] = (ctrlp[2] - ctrlp[0]) / span; | ||
slope[1] = (ctrlp[3] - ctrlp[1]) / span; | ||
tsReal step = span / (nsamples - 1); | ||
for(size_t i = 0; i < nsamples; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you know that TinySpline offers a function to sample splines: ts_bspline_sample
. It might be easier to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point - I completely forgot about that functionality even though I use it frequently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though, I'm leveraging the independent variable to confirm the slope, so I think I'll stick with ts_bspline_eval
for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point.
Happy to help, but I'm not sure what the expected behavior is for the failing unit test. The failure message is
Here's a quick link to the test: Line 434 in ba11df8
|
Ah, I think I understand - the multiplicity is what was throwing me off. I think the commit above is an appropriate fix. |
In this case, discontinuity must be ignored by |
This looks very good. It shows that the expected slopes still correspond to the intuitive results. I wonder why scaling (0.7 and 0.3) is necessary. Edit: Ok, now I understand. |
I really like that there are no longer any special cases for degree > 0. |
test/c/derive.c
Outdated
@@ -173,8 +194,8 @@ void derive_single_line_with_custom_knots(CuTest *tc) | |||
(int) ts_bspline_num_control_points(&spline)); | |||
TS_CALL(try, status.code, ts_bspline_control_points( | |||
&spline, &ctrlp, &status)) | |||
CuAssertDblEquals(tc, 2.f, ctrlp[0], EPSILON); | |||
CuAssertDblEquals(tc, 8.f, ctrlp[1], EPSILON); | |||
CuAssertDblEquals(tc, 1.f, ctrlp[0], EPSILON); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be easier to understand if you assert 2.f/2.f
and 8.f/2.f
(similar to derive_discontinuous_lines_ignoring_epsilon
). Dividing by 2.f
is necessary because span
is 2
.
The logic in derive was incorrectly avoiding scaling control points for
degree == 1
. This removes the unnecessary logic and updates thederive_single_line_with_custom_knots
unit test to demonstrate successful computation of the correct derivatives.This is not yet ready to merge as the
derive_discontinuous_lines_ignoring_epsilon
unit test is currently failing.