-
Notifications
You must be signed in to change notification settings - Fork 9
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 support for 1D physical space cases #75
Conversation
Hi @ovanvincq thank you very much for this contribution! It's nice to see people expanding the capabilities of this package. |
@JordiManyer I'm sorry but I don't see any comments in the 'conversation' tab or in the 'files changed' tab. I'm not familiar with Github but you are tagged as a pending reviewer. |
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.
Ok, I think the review was pending. You should see my comments now.
src/recipes.jl
Outdated
(x,y) | ||
end | ||
|
||
function Makie.convert_arguments(::Union{Type{Makie.Lines},Type{Makie.ScatterLines},Type{Makie.Scatter}}, trian::Union{Triangulation{0,1},Triangulation{1,1}}) |
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.
Use type parameters here
Union{Triangulation{0,1},Triangulation{1,1}}
-> Triangulation{Dc,1}, ...) where Dc
src/recipes.jl
Outdated
@@ -138,7 +138,7 @@ function Makie.plot!(p::MeshField{<:Tuple{Triangulation, Any}}) | |||
) | |||
end | |||
|
|||
Makie.plottype(::Triangulation, ::Any) = MeshField | |||
Makie.plottype(t::Triangulation, ::Any) = num_point_dims(t)==1 ? Makie.Scatter : MeshField |
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.
I would use dispatching here, i.e
Makie.plottype(t::Triangulation{Dc,1}, ::Any) where Dc = Makie.Scatter
Makie.plottype(t::Triangulation{Dc,Dp}, ::Any) where {Dc,Dp} = MeshField
src/recipes.jl
Outdated
@@ -109,7 +109,7 @@ function Makie.convert_arguments(t::Type{<:Union{Makie.Wireframe, Makie.Scatter} | |||
end | |||
|
|||
# Set default plottype as mesh if argument is type Triangulation, i.e., mesh(Ω) == plot(Ω). | |||
Makie.plottype(::Triangulation) = PlotGridMesh | |||
Makie.plottype(t::Triangulation) = num_point_dims(t)==1 ? Makie.Scatter : PlotGridMesh |
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.
Same comment: Use dispatching on Dc
and Dp
.
src/conversions.jl
Outdated
@@ -178,3 +178,10 @@ end | |||
|
|||
to_scalar(x) = x | |||
to_scalar(x::VectorValue) = norm(x) | |||
|
|||
function to_point1D(trian::Triangulation, uh::Any) |
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.
Maybe something like this would be more readable?
function to_point1D(trian::Triangulation{Dc,1}, uh::Any) where Dc
vds = first(visualization_data(trian, "", cellfields=["uh"=>uh]))
y = vds.nodaldata["uh"]
x = map(y->y[1], get_node_coordinates(vds.grid))
x, y
end
src/recipes.jl
Outdated
function Makie.convert_arguments(::Union{Type{Makie.Lines},Type{Makie.ScatterLines},Type{Makie.Scatter}}, c::CellField) | ||
if num_point_dims(get_triangulation(c))==1 | ||
trian=get_triangulation(c) | ||
data = to_point1D(trian, c) |
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.
Can we just do this?
x, y = to_point1D(trian, c)
src/recipes.jl
Outdated
(x,y) | ||
end | ||
|
||
Makie.plottype(c::CellField) = num_point_dims(get_triangulation(c))==1 ? Makie.Scatter : MeshField |
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.
This looks to me like we could do
Makie.plottype(c::CellField) = Makie.plottype(get_triangulation(c))
and it would be cleaner that repeating the logic we already have for triangulations.
Cleaner code
@JordiManyer Thank you for your advice. I made the corrections and the code is now cleaner. |
Merged! Thank you again for contributing! |
I add support for 1D physical space with examples and tests.
In the 1D physical space case, the
plot
function returns a curve describing the variations of the field as a function of the position.Support for 2D & 3D physical space cases is unchanged.
@JordiManyer If you have time, could you review this PR and tell me how I can improve it?