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

Non-breaking internal restructuring #79

Merged
merged 39 commits into from
Sep 23, 2024
Merged

Non-breaking internal restructuring #79

merged 39 commits into from
Sep 23, 2024

Conversation

mikeingold
Copy link
Collaborator

@mikeingold mikeingold commented Sep 22, 2024

Completed

  • Major internal re-stucturing and re-organization
    • Moved IntegrationAlgorithms from integrals.jl to a new file
    • Consolidated all non-specialized _integral workers into integrals.jl
    • Moved specialized geometry-specific methods from integral_*.jl files into geometry-specific files under the new /src/specializations/ directory
    • Made _integral the first-line worker for all integrals; the only remaining _integral_Nd methods are subordinate to _integral(f, ::GaussKronrod) methods
    • Culled some obsoleted and redundant method definitions
  • Generalized GaussLegendre integrals to a single n-dimensional method
  • Allow differential and jacobian to accept argument ts as a Tuple
  • Marginally loosened a few test tolerances (rtol=1e-6) to mitigate occasional test failures
  • Bump package version from v0.13.4 to v0.13.5 and remove sole author credit

Note on test coverage: the Ring, Rope, and BezierCurve methods identified as lacking coverage already existed and were only moved to new files in this branch. Ordinarily I would use this opportunity to define tests for them, but the affected methods are going to be obsoleted in an upcoming PR that converts FP to a keyword argument.

Future Work

  • Convert FP to a keyword argument.
    • Figure out how to ensure clean pass-through of arguments from alias functions, e.g. lineintegral -> integral -> _integral?
  • Rename IntegrationAlgorithm to IntegrationRule, change references from "algorithm", "settings", etc to "rule"
  • Add "Why Specialized" lines to each specialization source file: why does this geometry require specialized methods vs the generalized ones?

Copy link

codecov bot commented Sep 22, 2024

Codecov Report

Attention: Patch coverage is 93.19372% with 13 lines in your changes missing coverage. Please review.

Project coverage is 90.35%. Comparing base (7674006) to head (1e997a4).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/specializations/Ring.jl 50.00% 4 Missing ⚠️
src/specializations/Rope.jl 50.00% 4 Missing ⚠️
src/integral.jl 86.95% 3 Missing ⚠️
src/specializations/BezierCurve.jl 90.90% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #79      +/-   ##
==========================================
- Coverage   90.93%   90.35%   -0.59%     
==========================================
  Files           7       16       +9     
  Lines         353      311      -42     
==========================================
- Hits          321      281      -40     
+ Misses         32       30       -2     

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

@mikeingold mikeingold changed the title Breaking changes for v0.14 Non-breaking internal restructuring Sep 23, 2024
@mikeingold
Copy link
Collaborator Author

@JoshuaLampert Just a heads-up: I was planning for this branch/PR to become a major version bump, but the first (non-breaking) half of the job wound up being so extensive that I think I'm going to split those plans into two PR's. This reorganization is mostly moving methods into different files, combining some redundant wrapper methods, and implementing a generalized Gauss-Legendre method. I'll need to look over everything again, but it should probably be ready for review in the next day or two.

@mikeingold mikeingold marked this pull request as ready for review September 23, 2024 04:27
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.

Nice! I like having more structure in the code. I didn't check everything in detail (especially I assume the copy/paste is fine). I just left some minor comments and questions below.

src/integration_algorithms.jl Outdated Show resolved Hide resolved
src/integration_algorithms.jl Outdated Show resolved Hide resolved
test/Project.toml Outdated Show resolved Hide resolved
test/Project.toml Outdated Show resolved Hide resolved
src/integral.jl Show resolved Hide resolved
test/Project.toml Outdated Show resolved Hide resolved
src/integral.jl Show resolved Hide resolved
src/integral.jl Show resolved Hide resolved
@mikeingold mikeingold merged commit 8d5f81a into main Sep 23, 2024
11 of 13 checks passed
@mikeingold mikeingold deleted the v0.14 branch September 23, 2024 23:05
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