JuliaApproximation / DomainSets.jl

A Julia package for describing domains as continuous sets of elements
MIT License
72 stars 12 forks source link

Allow overloading f(::UnitSphere) #70

Closed dlfivefifty closed 3 years ago

dlfivefifty commented 3 years ago

Without this change one needs to do f(::UnitSphere{T}) where T

codecov[bot] commented 3 years ago

Codecov Report

Merging #70 (9f95651) into master (659b385) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #70   +/-   ##
=======================================
  Coverage   72.44%   72.44%           
=======================================
  Files          25       25           
  Lines        1154     1154           
=======================================
  Hits          836      836           
  Misses        318      318           
Impacted Files Coverage Δ
src/domains/ball.jl 75.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 659b385...9f95651. Read the comment docs.

daanhb commented 3 years ago

I think I'm missing the point here, can you expand? Probably the change should be made both for unit balls and unit spheres?

While we're at it, we could also use <:StaticArray{Tuple{N},T,1} type, rather than only an SVector to make the code more generic. In fact that might be an idea for all "euclidean" domains. Would that work?

dlfivefifty commented 3 years ago

I mean if I introduce a function like:

function foo(A::UnitSphere)
# do something
end

it won't actually be called without this PR.

While we're at it, we could also use <:StaticArray{Tuple{N},T,1} type, rather than only an SVector to make the code more generic. In fact that might be an idea for all "euclidean" domains. Would that work?

Yes, good idea, especially since I'm using SphericalCoordinate <: StaticVector{3} in https://github.com/JuliaApproximation/HarmonicOrthogonalPolynomials.jl/blob/c9afdb3ff921d532a81d0f5440e2a7bc60ccde8f/src/SphericalHarmonics.jl#L134

daanhb commented 3 years ago

Alright, I'll make a separate issue for supporting other static arrays.

Still missing the issue with unit spheres:

julia> b = UnitSphere()
the 3-dimensional unit sphere

julia> f(::UnitSphere) = 22
f (generic function with 1 method)

julia> f(b)
22

julia> g(::DomainSets.UnitHyperSphere) = 33
g (generic function with 1 method)

julia> g(b)
33

I guess the issue arises with some more involved definitions?

dlfivefifty commented 3 years ago

hmm I ran into it here:

https://github.com/JuliaApproximation/HarmonicOrthogonalPolynomials.jl/blob/c9afdb3ff921d532a81d0f5440e2a7bc60ccde8f/src/SphericalHarmonics.jl#L140

maybe you need 2 arguments?

daanhb commented 3 years ago

Minimal example, still seems to work here:

julia> using DomainSets, StaticArrays

julia> abstract type AbstractSphericalCoordinate{T} <: StaticVector{3,T} end

julia> struct SphericalCoordinate{T} <: AbstractSphericalCoordinate{T}
           θ::T
           φ::T
       end

julia> Base.in(::AbstractSphericalCoordinate, ::UnitSphere) = true

julia> function Base.getindex(S::SphericalCoordinate, k::Int)
           k == 1 && return sin(S.θ) * cos(S.φ)
           k == 2 && return sin(S.θ) * sin(S.φ)
           k == 3 && return cos(S.θ)
           throw(BoundsError(S, k))
       end

julia> s = SphericalCoordinate(0.1, 0.2)
3-element SphericalCoordinate{Float64} with indices SOneTo(3):
 0.09784339500725571
 0.019833838076209875
 0.9950041652780258

julia> s ∈ UnitSphere()
true

The pull request actually changes HyperBall and not HyperSphere, but that could be a mistake. I'm not objecting to any change, it is just that <:SVector{N,T} seems strange because SVector{N,T} is a concrete type.

daanhb commented 3 years ago

Hmm, maybe it is due to changes on my master branch that are not in a release.

dlfivefifty commented 3 years ago

Hmm.. weird, I can't reproduce the issue now...