Use a new DifferentiationMethod type for specifying derivative methods #122

mikeingold · 2024-10-26T02:56:15Z

Changes

Add a diff_method keyword argument to the integral API with a corresponding DifferentiationMethod type set for specifying how Jacobians should be calculated.
- FiniteDifference{T=Float64}(T(1e-6)) requests a finite difference approximation with the provided epsilon (step size) value.
- Analytical() requests usage of analytical solutions to derivatives.
- Added commented-out placeholders for AutoEnzyme and AutoZygote, to be defined in the future.
Changes function signatures for differential and jacobian to 3-argument: (geometry, ts, diff_method).
Adds non-exported internal utilities:
- _has_analytical(geometry) returns whether a geometry has a method defined for jacobian(geometry, ts, ::Analytical)
- _default_method(geometry) returns the recommended default DifferentiationMethod for a geometry, currently: Analytical() for geometries where they're defined, otherwise FiniteDifference()
- _guarantee_analytical(G, diff_method) is a hopefully-temporary workaround that throws a descriptive ArgumentError if _has_analytical(G) is false. Ref Use new Parametric Geometry types for internal domain transformations #128
Significant update to Documenter site: https://juliageometry.github.io/MeshIntegrals.jl/previews/PR122/
Bumps version number to v0.16.0-DEV (targeting a new version number pending other PR's merging).

codecov · 2024-10-26T03:34:40Z

Codecov Report

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

Project coverage is 95.86%. Comparing base (05238b2) to head (454ab43).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/specializations/Tetrahedron.jl	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #122      +/-   ##
==========================================
+ Coverage   95.26%   95.86%   +0.59%     
==========================================
  Files          17       17              
  Lines         380      290      -90     
==========================================
- Hits          362      278      -84     
+ Misses         18       12       -6

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

github-actions · 2024-10-26T03:34:53Z

Benchmark Results

	main	`454ab43`...	main/454ab43c8057d6...
Differentials/Differential	0.713 ± 0.044 μs	0.242 ± 0.0011 μs	2.94
Differentials/Jacobian	0.67 ± 0.0051 μs	0.206 ± 0 μs	3.25
Integrals/Meshes.BezierCurve/Scalar GaussKronrod	23 ± 0.097 ms	22 ± 0.073 ms	1.04
Integrals/Meshes.BezierCurve/Scalar GaussLegendre	22 ± 0.077 ms	21.1 ± 0.061 ms	1.04
Integrals/Meshes.BezierCurve/Scalar HAdaptiveCubature	23.2 ± 0.088 ms	22.2 ± 0.039 ms	1.04
Integrals/Meshes.BezierCurve/Vector GaussKronrod	23 ± 0.075 ms	22 ± 0.045 ms	1.04
Integrals/Meshes.BezierCurve/Vector GaussLegendre	22 ± 0.049 ms	21.1 ± 0.04 ms	1.04
Integrals/Meshes.BezierCurve/Vector HAdaptiveCubature	23.2 ± 0.075 ms	22.2 ± 0.046 ms	1.04
Integrals/Meshes.Segment/Scalar GaussKronrod	4.37 ± 0.25 μs	1.19 ± 0.006 μs	3.67
Integrals/Meshes.Segment/Scalar GaussLegendre	0.0751 ± 0.0007 ms	0.0542 ± 0.0007 ms	1.38
Integrals/Meshes.Segment/Scalar HAdaptiveCubature	8.06 ± 0.081 μs	4.69 ± 0.06 μs	1.72
Integrals/Meshes.Segment/Vector GaussKronrod	6.63 ± 0.07 μs	3.43 ± 0.056 μs	1.93
Integrals/Meshes.Segment/Vector GaussLegendre	0.0888 ± 0.00086 ms	0.0653 ± 0.0007 ms	1.36
Integrals/Meshes.Segment/Vector HAdaptiveCubature	11.1 ± 0.28 μs	7.62 ± 0.13 μs	1.46
Integrals/Meshes.Sphere/Scalar GaussKronrod	0.197 ± 0.00081 ms	0.0863 ± 0.00049 ms	2.28
Integrals/Meshes.Sphere/Scalar GaussLegendre	18 ± 0.14 ms	13.4 ± 0.1 ms	1.34
Integrals/Meshes.Sphere/Scalar HAdaptiveCubature	0.189 ± 0.0025 ms	0.0639 ± 0.00034 ms	2.96
Integrals/Meshes.Sphere/Vector GaussKronrod	0.231 ± 0.0019 ms	0.118 ± 0.00092 ms	1.95
Integrals/Meshes.Sphere/Vector GaussLegendre	19.3 ± 0.28 ms	14.9 ± 0.25 ms	1.3
Integrals/Meshes.Sphere/Vector HAdaptiveCubature	0.243 ± 0.0018 ms	0.115 ± 0.0011 ms	2.12
time_to_load	1.63 ± 0.0064 s	1.64 ± 0.007 s	0.994

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

src/differentiation.jl

Co-authored-by: Joshua Lampert <[email protected]>

JoshuaLampert

I would also export FiniteDifference. We could also think about a backend Analytical or similar, which computes the analytical derivative, which would currently be only supported for a BezierCurve, but that's fine IMO. What do you think?

src/differentiation.jl

src/integral.jl

Co-authored-by: Joshua Lampert <[email protected]>

JoshuaLampert · 2024-10-26T14:57:28Z

This is breaking because, e.g., jacobian(geometry, ts; ε=1e-7), doesn't work anymore.

mikeingold · 2024-10-26T15:12:37Z

This is breaking because, e.g., jacobian(geometry, ts; ε=1e-7), doesn't work anymore.

Yes. I’ve been planning for this to be part of a v0.16 release. That change is breaking and this new support structure around specifying diff type is a non-trivial feature update anyway.

JoshuaLampert · 2024-10-26T15:15:25Z

Yeah, I'm fine with that 👍

src/utils.jl

mikeingold · 2024-11-06T00:28:42Z

Looks like the improvement came with commit #f845289 where the jacobian type params for ts changed from Tuple to NTuple. I'm not totally sure why that's worth a 3x speed boost, but I'll take it.

I suspect this is simply because NTuple additionally guarantees that all elements are of identical type.

mikeingold · 2024-11-06T00:36:03Z

Status update

Remaining TODO:

We're currently failing an Aqua test for unbound type parameters but I'm not entirely clear on why.
Documentation build is apparently failing due to presence of unicode.
I have one more minor tweak to the FiniteDifference version of jacobian that I want to try. I suspect we might get some additional performance out of it.

kylebeggs · 2024-11-06T01:00:35Z

I suspect this is simply because NTuple additionally guarantees that all elements are of identical type.

That is something I haven't thought about, but it makes sense! Thanks for all the work on this, it is shaping up wonderfully.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Co-authored-by: Joshua Lampert <[email protected]>

mikeingold · 2024-11-06T02:38:10Z

The failing Aqua test for unbound params in jacobian is a puzzle for me. The code functions correctly, but technically N is unbound if v is a vector and not an NTuple. Not sure how else to express this in a way that still works nicely but also passes the test.

https://julialang.zulipchat.com/#narrow/channel/225542-helpdesk/topic/Aqua.20unbound.20type.20parameter.20on.20Union

JoshuaLampert

I have a few smaller suggestions and I left a question for discussion.

docs/src/how_it_works.md

src/differentiation.jl

src/specializations/Line.jl

Co-authored-by: Joshua Lampert <[email protected]>

mikeingold · 2024-11-06T16:31:53Z

The latest commit fixes the unbound parameters issue, so hopefully the newfound performance improvement is also maintained.

mikeingold · 2024-11-06T17:04:26Z

Looks like performance was maintained. I think we might be essentially done with this one, finally.

mikeingold · 2024-11-06T18:07:56Z

The OP post has been updated with a summary of all changes. Bumping one last time for any remaining review notes/suggestions.

docs/src/how_it_works.md

src/integral.jl

src/specializations/BezierCurve.jl

Co-authored-by: Joshua Lampert <[email protected]>

docs/src/specializations.md

docs/src/how_it_works.md

Co-authored-by: Joshua Lampert <[email protected]>

JoshuaLampert

Thanks!

mikeingold added 9 commits October 25, 2024 22:35

Add JacobianMethod types

ce097f6

Change JacobianMethod to DifferentiationMethod

3decf9f

Add a third argument to jacobian for DiffMethod for dispatch

bd54b31

Remove dead line

4cb6b02

Remove obsolete analytic jacobian for BezierCurve

34fbca2

Add default step size if unspecified

7e5a405

Convert differential to three-argument with default FiniteDiff method

f17f326

Bigfix

ac23e43

Add two-arg Jacobian pointing to FD

ceca1b5

Apply formatting suggestion

2e7b23f

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

JoshuaLampert reviewed Oct 26, 2024

View reviewed changes

src/differentiation.jl Outdated Show resolved Hide resolved

mikeingold and others added 5 commits October 26, 2024 07:33

Apply suggestion

bfab883

Co-authored-by: Joshua Lampert <[email protected]>

Add a dt argument

62d7a53

Update docstring API

3114e54

Add dt kwargs

3f0df9a

Add missing params

8d7753d

JoshuaLampert reviewed Oct 26, 2024

View reviewed changes

src/differentiation.jl Outdated Show resolved Hide resolved

src/integral.jl Outdated Show resolved Hide resolved

mikeingold and others added 5 commits October 26, 2024 08:57

Add dt kwargs (with TODOs)

855f3a9

Consolidate kwarg naming

d832b05

Update src/differentiation.jl

6935da6

Co-authored-by: Joshua Lampert <[email protected]>

Add exports

bfcd204

Tweak suggested naming [skip CI]

80b05f9

JoshuaLampert added the refactoring label Oct 26, 2024

Update Line methods to use an internal differential method

323c5d0

JoshuaLampert added the breaking label Oct 26, 2024

github-actions bot reviewed Nov 6, 2024

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

mikeingold added 2 commits November 5, 2024 19:41

Test externalizing partial derivative function

dab1e6b

Re-internalize partial derivative function

6d96b63

mikeingold and others added 3 commits November 5, 2024 20:01

Remove errant character

d2fd7f4

Apply format suggestions [skip ci]

84d8db2

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Apply suggestions from code review [skip ci]

49a5195

Co-authored-by: Joshua Lampert <[email protected]>

JoshuaLampert requested changes Nov 6, 2024

View reviewed changes

docs/src/how_it_works.md Show resolved Hide resolved

docs/src/how_it_works.md Outdated Show resolved Hide resolved

docs/src/how_it_works.md Show resolved Hide resolved

src/differentiation.jl Outdated Show resolved Hide resolved

src/specializations/Line.jl Outdated Show resolved Hide resolved

mikeingold and others added 2 commits November 6, 2024 08:55

Apply suggestions from code review

8a8ec7a

Co-authored-by: Joshua Lampert <[email protected]>

Try new parameterization

fd263eb

Remove obsolete comment

1f03665

mikeingold marked this pull request as ready for review November 6, 2024 17:04

mikeingold requested review from JoshuaLampert and kylebeggs November 6, 2024 18:08

JoshuaLampert requested changes Nov 6, 2024

View reviewed changes

mikeingold and others added 2 commits November 6, 2024 21:25

Apply suggestions from code review

671d4e2

Co-authored-by: Joshua Lampert <[email protected]>

Consolidate information on specializations

079e0c6

JoshuaLampert reviewed Nov 7, 2024

View reviewed changes

docs/src/specializations.md Outdated Show resolved Hide resolved

docs/src/how_it_works.md Outdated Show resolved Hide resolved

mikeingold and others added 2 commits November 7, 2024 14:06

Apply suggestion

12b62ee

Apply suggestion

454ab43

Co-authored-by: Joshua Lampert <[email protected]>

JoshuaLampert approved these changes Nov 8, 2024

View reviewed changes

mikeingold merged commit 49aaf57 into main Nov 8, 2024
12 checks passed

mikeingold deleted the diff-select branch November 8, 2024 13:03

mikeingold mentioned this pull request Nov 12, 2024

add AD (Enzyme) support via EnzymeExt #129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a new DifferentiationMethod type for specifying derivative methods #122

Use a new DifferentiationMethod type for specifying derivative methods #122

mikeingold commented Oct 26, 2024 •

edited

Loading

codecov bot commented Oct 26, 2024 •

edited

Loading

github-actions bot commented Oct 26, 2024 •

edited

Loading

JoshuaLampert left a comment

JoshuaLampert commented Oct 26, 2024

mikeingold commented Oct 26, 2024

JoshuaLampert commented Oct 26, 2024

mikeingold commented Nov 6, 2024

mikeingold commented Nov 6, 2024 •

edited

Loading

kylebeggs commented Nov 6, 2024 •

edited

Loading

mikeingold commented Nov 6, 2024

JoshuaLampert left a comment

mikeingold commented Nov 6, 2024

mikeingold commented Nov 6, 2024

mikeingold commented Nov 6, 2024

JoshuaLampert left a comment

Use a new DifferentiationMethod type for specifying derivative methods #122

Use a new DifferentiationMethod type for specifying derivative methods #122

Conversation

mikeingold commented Oct 26, 2024 • edited Loading

Changes

codecov bot commented Oct 26, 2024 • edited Loading

Codecov Report

github-actions bot commented Oct 26, 2024 • edited Loading

Benchmark Results

Benchmark Plots

JoshuaLampert left a comment

Choose a reason for hiding this comment

JoshuaLampert commented Oct 26, 2024

mikeingold commented Oct 26, 2024

JoshuaLampert commented Oct 26, 2024

mikeingold commented Nov 6, 2024

mikeingold commented Nov 6, 2024 • edited Loading

Status update

kylebeggs commented Nov 6, 2024 • edited Loading

mikeingold commented Nov 6, 2024

JoshuaLampert left a comment

Choose a reason for hiding this comment

mikeingold commented Nov 6, 2024

mikeingold commented Nov 6, 2024

mikeingold commented Nov 6, 2024

JoshuaLampert left a comment

Choose a reason for hiding this comment

mikeingold commented Oct 26, 2024 •

edited

Loading

codecov bot commented Oct 26, 2024 •

edited

Loading

github-actions bot commented Oct 26, 2024 •

edited

Loading

mikeingold commented Nov 6, 2024 •

edited

Loading

kylebeggs commented Nov 6, 2024 •

edited

Loading