-
Notifications
You must be signed in to change notification settings - Fork 94
Add ParametrizedCurve
#1093
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
Add ParametrizedCurve
#1093
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1093 +/- ##
==========================================
+ Coverage 87.40% 87.44% +0.04%
==========================================
Files 189 190 +1
Lines 5962 5983 +21
==========================================
+ Hits 5211 5232 +21
Misses 751 751 ☔ View full report in Codecov by Sentry. |
juliohm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate if you can elaborate on the use cases of this primitive so that we can fine tune the best constructors.
If we restrict t in [0,1] and constrain the type to Cartesian parametrizations, we can think of constructing the curve from parametric x(t), y(t) (and possibly z(t)) alone. Otherwise, we can consider other intervals for t and other CRS like Polar, Spherical, Cylindrical coordinates in which case, the parametrizations would be r(t), theta(t), etc.
|
Thanks a lot for your feedback @juliohm. I structure your comments into two points:
One use case I have in mind is the following. I would like to compute integrals along non-trivial curves in 2D like the one in Figure 7 of https://academic.oup.com/imajna/article/43/3/1616/6586010. Looking at the source code of this paper the curve can be described by the following curve in polar coordinates: f(theta) = 1 - 1/3 * sin(2*theta)^2
curve = ParametrizedCurve(-pi, pi, t -> Point(Polar(f(t), t)))The following already works with the current implementation, which is good (but not as nice as the above, see point 1): curve = ParametrizedCurve(0.0, 1.0, t -> Point(Polar(f(2pi * t - pi), 2pi * t - pi))) |
This is a tricky question. All of Meshes.jl's current parametric functions, and other features that utilize them, assume an interface with domain If we maintained support for
|
|
The current approach where the interval [a,b] is handled internally in the struct is flexible and doesn't compromise the assumption that uv coords are unitless coordinates in the range [0,1]. Perhaps we just need to make this interval [a,b] something of secondary importance with [0,1] as the default. What about the following constructor: ParametrizedCurve(f, ab=(0.0, 1.0))That way you could write the example as f(theta) = 1 - 1/3 * sin(2*theta)^2
# explicit ab interval
ParametrizedCurve(t -> Point(Polar(f(t), t)), (-pi, pi))
# default interval
ParametrizedCurve(t -> Point(Polar(f(2pi * t - pi), 2pi * t - pi))) |
|
Sounds good @juliohm. I pushed your suggestion. |
juliohm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor suggestions to consider before we start adding tests and docs.
|
Not to overly complicate this suggestion, but I'll just throw this out
there...
If we want to define a one-dimensional geometry defined by user-supplied
parametric functions, it seems logical that we might consider extending
this in the future to also accommodate parametric surfaces and volumes. If
we were to go that far, would we need to define new types, e.g.
ParametricSurface, or could these all fall under the same basic type, e.g.
Box?
…On Tue, Oct 1, 2024 at 9:28 AM Júlio Hoffimann ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Just a few minor suggestions to consider before we start adding tests and
docs.
------------------------------
In src/geometries/primitives.jl
<#1093 (comment)>
:
> @@ -31,3 +31,4 @@ include("primitives/frustum.jl")
include("primitives/frustumsurface.jl")
include("primitives/paraboloidsurface.jl")
include("primitives/torus.jl")
+include("primitives/parametrizedcurve.jl")
please move this include to after bezier.jl
------------------------------
In src/geometries/primitives/parametrizedcurve.jl
<#1093 (comment)>
:
> +A parametrized curve is a curve defined by a function `func` that maps a parameter `t` to a `Point` in space.
+The parameter `t` is defined in the interval `[a, b]`. The curve can only be evaluated for `t` in the
+interval `[0, 1]`.
⬇️ Suggested change
-A parametrized curve is a curve defined by a function `func` that maps a parameter `t` to a `Point` in space.
-The parameter `t` is defined in the interval `[a, b]`. The curve can only be evaluated for `t` in the
-interval `[0, 1]`.
+A curve defined by a function `func` that maps a parameter `t` in the real line to a
+`Point` in space. The parameter `range` can be specified, and defaults to `[0,1]` otherwise.
------------------------------
In src/geometries/primitives/parametrizedcurve.jl
<#1093 (comment)>
:
> @@ -0,0 +1,48 @@
+# ------------------------------------------------------------------
+# Licensed under the MIT License. See LICENSE in the project root.
+# ------------------------------------------------------------------
+
+"""
+ ParametrizedCurve(func, ab = (0.0, 1.0))
⬇️ Suggested change
- ParametrizedCurve(func, ab = (0.0, 1.0))
+ ParametrizedCurve(func, range = (0.0, 1.0))
------------------------------
In src/geometries/primitives/parametrizedcurve.jl
<#1093 (comment)>
:
> + a::T
+ b::T
+ func::F
Let's store the fields func::F and range::R with F<:Function and R<:Tuple
in that order, to match the constructor.
------------------------------
In src/geometries/primitives/parametrizedcurve.jl
<#1093 (comment)>
:
> + a::T
+ b::T
+ func::F
+
+ function ParametrizedCurve(func, ab=(0.0, 1.0))
+ a, b = promote(ab...)
+ p = func(a)
+ M = manifold(p)
+ C = crs(p)
+ T = typeof(a)
+ new{M,C,T,typeof(func)}(a, b, func)
+ end
+end
+
+paramdim(::ParametrizedCurve) = 1
+startparameter(curve::ParametrizedCurve) = curve.a
startparameter and endparameter need to be exposed as part of the public
API? I was thinking that once created, the curve would be evaluated with
the unit interval range.
If these are needed, we can consider adding a method Base.range(c::Curve)
= c.range[1]:c.range[2] instead.
------------------------------
In src/geometries/primitives/parametrizedcurve.jl
<#1093 (comment)>
:
> + a = startparameter(curve)
+ b = endparameter(curve)
+ θ = a + t * (b - a)
+ curve.func(θ)
⬇️ Suggested change
- a = startparameter(curve)
- b = endparameter(curve)
- θ = a + t * (b - a)
- curve.func(θ)
+ a, b = curve.range
+ curve.func(a + t * (b - a))
—
Reply to this email directly, view it on GitHub
<#1093 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA657ORQXBXSDO6VCFFAFUDZZKPRRAVCNFSM6AAAAABPBQN6XSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNBQGIYTMOBWGM>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
I'll address your suggestions once I am back at my workstation @juliohm. |
|
Agreed. I don't want this suggestion to halt progress on this effort, but
if we're interested in extending this work to more dimensions then it might
be worth having a roadmap in mind to avoid breaking changes.
It's not unprecedented for one name to operate in different numbers of
dimensions, e.g. Box, Sphere, and Ball, that might be used for inspiration.
The only real difference between a parametric curve and surfaces/volumes
would be the number of input arguments to the parametric function, and the
sizes of `a` and `b`.
…On Tue, Oct 1, 2024 at 1:20 PM Joshua Lampert ***@***.***> wrote:
I'll address your suggestions once I am back at my workstation @juliohm
<https://github.com/juliohm>.
Thanks for starting the discussion about higher dimensional user-supplied
parametric functions @mikeingold <https://github.com/mikeingold>. I also
thought about that, but assumed we would need different types for that. So
I wanted to postpone that for now. But you might be right that it could be
good to just have the same struct for all parametric dimensions, which
means we should discuss it already in this PR to avoid breaking changes. I
would be interested in your thoughts, @juliohm
<https://github.com/juliohm>.
—
Reply to this email directly, view it on GitHub
<#1093 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA657OQPV5O5YKVEYAFKQ7LZZLKXTAVCNFSM6AAAAABPBQN6XSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBWGU2TQOJQGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>
Very good point. I wonder what is the best design here. We are trying to achieve a continuous representation with any number of parametric coordinates If we think of the ranges of the parameters |
|
I think I have an idea. The function f(c::Cartesian)
x, y = c.x, c.y
Polar(x, x / y)
endThe Maybe that will cover the use cases you have in mind? Should we experiment with the |
|
Sounds interesting. I'll need to think about it and experiment a bit (I'm currently on vacation, so I might need a bit of time). |
|
In #1100 we came to the conclusion that having a new |
…/Meshes.jl into parametrized-curve
|
Thank you @JoshuaLampert, these are great additions. I will try to find some time to write a section in the docs regarding the uv coords of geometries. |
|
That would be great! Thanks a lot for your valuable input and review. |
…/Meshes.jl into parametrized-curve
|
I added tests. The only test failure seems unrelated to this PR. |
juliohm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor adjustments regarding machine type of literals.
Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com>
Co-authored-by: Elias Carvalho <73039601+eliascarv@users.noreply.github.com>
|
Thank you @JoshuaLampert ! Releasing a patch... |
Now I see it's documented in https://juliageometry.github.io/MeshesDocs/stable/predicates/#Meshes.isparametrized, but a more prominent documentation could still be helpful (but is not urgent). |
* add ParametrizedCurve * include file * add docstring * format * remove underscores in names * use minimum and maximum * fix * interval ab as optional argument * format * move include * Apply suggestions from code review Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com> * restructure range * Apply suggestions from code review Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com> * promote range * Update src/geometries/primitives/parametrizedcurve.jl * Update src/geometries/primitives/parametrizedcurve.jl * Update src/geometries/primitives/parametrizedcurve.jl Co-authored-by: Joshua Lampert <51029046+JoshuaLampert@users.noreply.github.com> * add docs * Update src/geometries/primitives/parametrizedcurve.jl Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com> * remove newline * fix Float32 tests * add predicates test * add crs test * add sampling test * add discretization tests * format * use cart and merc consistently * Apply suggestions from code review Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com> * Update src/geometries/primitives/parametrizedcurve.jl Co-authored-by: Elias Carvalho <73039601+eliascarv@users.noreply.github.com> * func -> fun --------- Co-authored-by: Júlio Hoffimann <julio.hoffimann@gmail.com> Co-authored-by: Elias Carvalho <73039601+eliascarv@users.noreply.github.com>

This PR adds another primitive for a parametrized curve$\gamma: [a, b]\to\mathbb{R}^d$ , where the parametrization comes from a user-defined function. Please tell me if I missed any method, which should be added. I'll add tests later after the interface is ironed out.