Add AdaptiveSampling and AdaptiveDiscretization for one-dimensional parametric geometries #1234
Conversation
Benchmark Results (Julia vlts)Time benchmarks
Memory benchmarks
|
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1234 +/- ##
==========================================
- Coverage 87.89% 87.62% -0.27%
==========================================
Files 196 198 +2
Lines 6189 6257 +68
==========================================
+ Hits 5440 5483 +43
- Misses 749 774 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Huh the 32 bit tests seem to fail for subtle numerical deviations |
|
So I'll jump in with an unofficial review on the simple-to-fix/broad stuff before Julio and Josh since I have the time. The code conventions are that variable names should be short, descriptive, and not use underscores unless it is an internal, private function, in which case the underscore starts the function name (i.e Other's can chime in on this, but we are planning on adding BinaryTrees.jl to this package, so that might be a way of creating your sorted sequence (currently BinaryHeap) without adding an extra datastructure dependency in the long term. As for the FP32, you concrete type |
|
💯% agree with @souma4 's review. Try to adjust the code for a second round of review @sschuldenzucker. There are code style adjustments and dependency adjustments to make. If all you need is a BinaryTree, then BinaryTrees.jl is a better (more lightweight) dependency. DataStructures.jl has too much in it. |
|
Adaptive sampling/discretization will be super useful for visualization and other purposes. Depending on the performance of the implementation compared with |
|
At some point I'll write up an update to the contribution document to give a push for what the style of this code base is, but scouring old messages and stuff I notice I get these. I don't see all of them from my quick review, but good to have them in one place.
|
|
Thanks for putting this list together @souma4 ! 🫶 I would also add that we try to follow the notation from scientific publications when that is possible. That way new contributors can read the paper and the implementation side by side to propose improvements more easily. |
|
Hi!
On As for the SOMEDAYs: The ones that are not in the docstring are mostly about code quality; happy for any pointers. For the ones in the docstring, these seem to sit at various levels of difficulty. Here's an annotated list. Would love to do more but there seem to be some tricky parts:
|
That is really unfortunate. DataStructures.jl is one of those packages that provides various incomplete implementations without a single active maintainer. I've encountered so many issues in the past that I realized it would be safer to just not use it at all. It seems that the dependency is coming from another poorly maintained package, which is doubly unfortunate: (jl_YsCiCv) pkg> why DataStructures
Meshes → StatsBase → DataStructures
Meshes → StatsBase → SortingAlgorithms → DataStructuresOur ultimate goal is to remove StatsBase.jl from the list of dependencies. It dates back to the origins of Julia when the Statistics stdlib was part of the Julia distribution and people were trying to add more stats functionality independently of the Julia release process. |
There was a problem hiding this comment.
Thank you @sschuldenzucker for attempting your first contribution! 🎉
We still have work to do in order to improve the quality of the code and adhere to the project standards. Please feel free to ask any question as you are working on the review comments.
| crv = ParametrizedCurve((t -> Point((sin(t * 2π) * t, cos(t * 2π) * t))), (0, 1)) | ||
| method = AdaptiveDiscretization(; min_points=10.0, max_points=100.0) | ||
| mesh = discretize(crv, method) | ||
|
|
||
| viz(mesh, showsegments = true) |
There was a problem hiding this comment.
Please rename crv to curve. Also, the keyword arguments have underscores, which is not adherent to our code style.
| # Sample between 10 and 100 points | ||
| sampler = AdaptiveSampling(; min_points=10.0, max_points=100.0) | ||
| crv = ParametrizedCurve((t -> Point((sin(t * 2π) * t, cos(t * 2π) * t))), (0, 1)) | ||
| points = sample(crv, sampler) |> collect | ||
|
|
||
| viz(points) |
There was a problem hiding this comment.
Same suggestions I shared in the other discretization example.
| using DataStructures: BinaryHeap | ||
| using LinearAlgebra: norm | ||
| import IterTools |
There was a problem hiding this comment.
The only place in the source code where we import modules is the main Meshes.jl file. That way we don't loose track of current imports and avoid naming conflicts.
| This is *only* implemented for geometries with a one-dimensional parameter domain (i.e., realistically, [`ParametrizedCurve`](@ref) or [`BezierCurve`](@ref))! | ||
|
|
There was a problem hiding this comment.
| This is *only* implemented for geometries with a one-dimensional parameter domain (i.e., realistically, [`ParametrizedCurve`](@ref) or [`BezierCurve`](@ref))! |
No need to make this information available in the docstring. At some point in the future, we hope to fill in the gaps.
| * `tol` - Tolerance for the linear interpolation error. The meaning depends on `errfun`. | ||
| * `errfun` - Function to compute the interpolation error. The default is [`errRelativeBBox`](@ref) = the l-infinity distance component-wise relative to (an estimate of) the range of the bounding box; this is a good default for plotting. If provided, it must satisfy `errfun(::ParametrizedSegment, ::MutableBox)::E` | ||
| * `minPoints` - Number of points sampled uniformly initially. | ||
| * `maxPoints` - Maximum number of points to sample. We do our best to, if this is hit, get rid of the worst approximation errors first. |
There was a problem hiding this comment.
Please revise the options as they follow a camel case notation, not supported by our code style. Also, it is worth thinking which of these options will be changed in practice. Do you expect end-users to change the errfun?
| - Is there some smartness we can do if the (second) derivative of f is available? Choosing the splitting point better than at the interval midpoint seems attractive. | ||
| """ | ||
| struct AdaptiveSampling{E} <: ContinuousSamplingMethod | ||
| tol::E |
There was a problem hiding this comment.
Are you assuming that the tolerance is expressed as a unitless floating point number or as a tolerance with length units? We usually adopt the latter in our methods because they are more useful in applications. A user might want to sample the curve until the deviation/error is smaller than 1mm for example.
| minPoints::Int | ||
| maxPoints::Int |
There was a problem hiding this comment.
Not compliant with our code style.
| struct ParametrizedSegment{T,V} | ||
| t1::T | ||
| v1::V | ||
| tMid::T | ||
| vMid::V | ||
| t2::T | ||
| v2::V | ||
| end |
There was a problem hiding this comment.
Why not use the Segment type? It is parametrized already.
| "Like `Box` but mutable." | ||
| mutable struct MutableBox{V} | ||
| # Implicit: All lengths are equal | ||
| mins::Vector{V} | ||
| maxs::Vector{V} | ||
| end |
There was a problem hiding this comment.
I don't think mutability is enough justification to create another box type.
| min_points = 5 | ||
| max_points = 10 |
|
ping @sschuldenzucker |
This uses the algorithm from here: https://github.com/sschuldenzucker/parametricadaptivesampling.jl/
Tested with
ParametricCurveandBezierCurve.