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

v0.14-DEV: Remove derivative and unitdirection from exports #88

Merged
merged 3 commits into from
Sep 25, 2024

Conversation

mikeingold
Copy link
Owner

@mikeingold mikeingold commented Sep 25, 2024

Completed

  • Removed derivative and unitdirection from exports. These functions were developed early on when the direction of this package wasn't fully clear, and only BezierCurve-specific methods were added. In the long-term, I'd like to implement a more robust system for calculating derivatives and Jacobians, ideally using automatic differentiation. I'm removing these from the public API to facilitate future experimentation but won't remove them from the package entirely.

@mikeingold mikeingold changed the title v0.14-DEV: Remove utilities from exports v0.14-DEV: Remove derivative and unitdirection from exports Sep 25, 2024
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.50%. Comparing base (d27fa22) to head (9c2cd91).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #88   +/-   ##
=======================================
  Coverage   90.50%   90.50%           
=======================================
  Files          16       16           
  Lines         316      316           
=======================================
  Hits          286      286           
  Misses         30       30           

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

@mikeingold mikeingold marked this pull request as ready for review September 25, 2024 17:38
@JoshuaLampert
Copy link
Collaborator

Why is jacobian still exported?

@mikeingold
Copy link
Owner Author

Why is jacobian still exported?

The derivative and unitdirection (unit vector in same direction as derivative) methods are only defined for Bézier curves and seem mostly like a half-baked idea that we’re never fully implemented. I think my plan was to define these methods for all geometries, but it became obvious to me that plan would be fragile to any upstream changes in how geometries are parameterized. Ideally we could use AD as a general solution for partial derivatives of all geometry types, but I’ve had no luck yet in getting that working out-of-the-box despite some enthusiastic declarations upstream.

For now, I wound up just implementing an inelegant but reasonably effective finite difference approximation within jacobian. I was also going to drop jacobian but decided that it was at least usable for different geometry types, interesting enough that people might be interested in using it directly, and had a reasonably-stable API. There’s definitely room for improvement, but I figured this one could stick around.

Copy link
Collaborator

@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.

Ok, thanks for the explanation. If you think jacobian is reasonably stable and could be useful in its own, I'm fine exporting it (although I think it's a bit tangential to the goal of the package).

@mikeingold mikeingold merged commit fb671d8 into main Sep 25, 2024
15 checks passed
@JoshuaLampert JoshuaLampert deleted the unexport branch September 25, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants