Skip to content

Conversation

@mikeingold
Copy link
Collaborator

@mikeingold mikeingold commented Sep 27, 2024

Completed

  • Merge derivative function into jacobian
  • Use differential inside Bezier curve integral methods
  • Add a new @testitem section for BezierCurve with analytic solution
  • Add jacobian to the Documenter site API page

@JoshuaLampert
Copy link
Member

I was just thinking: Could we rename derivative to jacobian, i.e. add a method to jacobian, such that we can reuse differential within the Bezier specialization instead of needing norm(derivative(...))? Then, of course, the new jacobian method (which is now derivative) would also get ts instead of t (where we would obtain t by t = only(ts)) and would need to return a one-element vector.

JoshuaLampert and others added 3 commits September 27, 2024 19:09
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@mikeingold
Copy link
Collaborator Author

Didn't see you were making edits and just pushed a couple of commits. I have to step away but will check back in a bit to try to sort it out.

@JoshuaLampert
Copy link
Member

Oh, I'm sorry. I didn't want to disturb your development 😬

@mikeingold
Copy link
Collaborator Author

No worries! I'm glad to have the help, just got distracted over in VS Code and was surprised to see help had arrived.

@mikeingold
Copy link
Collaborator Author

I just added a @testitem section with new tests that have an analytic solution. I think the core issue we see with test results being non-exact is that these Bezier curves are merely splined/interpolated approximations of the ideal curves. Depending on how the error tolerances stack up, I may need to just lax the rtol or atol a little.

@mikeingold
Copy link
Collaborator Author

I was just thinking: Could we rename derivative to jacobian, i.e. add a method to jacobian, such that we can reuse differential within the Bezier specialization instead of needing norm(derivative(...))? Then, of course, the new jacobian method (which is now derivative) would also get ts instead of t (where we would obtain t by t = only(ts)) and would need to return a one-element vector.

This is a really good idea. I think part of the trouble I had conceptually with extending (partial) derivative was in trying to generalize it past 1D geometries, i.e. what does the API look like to request the partial derivative w.r.t. the N'th parametric coordinate? Whereas for the 1D case you can just ignore that bit and return a simple Vec.

In my mind, then, we've got

jacobian(geometry::Meshes.Geometry, ts)  # finite-diff fallback
jacobian(curve::Meshes.BezierCurve, ts)  # analytic solution from today's derivative
# etc (as implemented)

and can interface them the same in all integral functions.

mikeingold and others added 4 commits September 27, 2024 15:54
Co-authored-by: Joshua Lampert <51029046+JoshuaLampert@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@mikeingold mikeingold changed the title Convert Bezier integrals from static differential to using derivative Implement differential and jacobian for Bezier curve integrals Sep 27, 2024
@mikeingold mikeingold marked this pull request as ready for review September 28, 2024 00:00
@mikeingold
Copy link
Collaborator Author

Pending this latest commit passing CI, this should be good to go.

@codecov
Copy link

codecov bot commented Sep 28, 2024

Codecov Report

Attention: Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.25%. Comparing base (d784136) to head (7d98fd2).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/differentiation.jl 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #92      +/-   ##
==========================================
+ Coverage   90.50%   99.25%   +8.75%     
==========================================
  Files          16       17       +1     
  Lines         316      269      -47     
==========================================
- Hits          286      267      -19     
+ Misses         30        2      -28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@JoshuaLampert JoshuaLampert left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @mikeingold!

@mikeingold mikeingold merged commit 79ea114 into main Sep 28, 2024
15 checks passed
@mikeingold mikeingold deleted the bezier-derivative branch September 28, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants