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

Add support for 1D physical space cases #75

Merged
merged 7 commits into from
Sep 21, 2024
Merged

Conversation

ovanvincq
Copy link
Contributor

@ovanvincq ovanvincq commented Sep 13, 2024

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?

@JordiManyer
Copy link
Member

Hi @ovanvincq thank you very much for this contribution! It's nice to see people expanding the capabilities of this package.
I've added some possible reviews, feel free to edit them as you see fit.
I'll merge this when you're done.

@ovanvincq
Copy link
Contributor Author

@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.

Copy link
Member

@JordiManyer JordiManyer left a 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}})
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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.

@@ -178,3 +178,10 @@ end

to_scalar(x) = x
to_scalar(x::VectorValue) = norm(x)

function to_point1D(trian::Triangulation, uh::Any)
Copy link
Member

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)
Copy link
Member

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
Copy link
Member

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.

@ovanvincq
Copy link
Contributor Author

@JordiManyer Thank you for your advice. I made the corrections and the code is now cleaner.

@JordiManyer JordiManyer merged commit 241070a into gridap:master Sep 21, 2024
2 checks passed
@JordiManyer
Copy link
Member

Merged! Thank you again for contributing!

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