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 the posibility to plot 2D surfaces in 3D #82

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

amrueda
Copy link

@amrueda amrueda commented Nov 21, 2023

This PR adds support for Trixi simulations on spherical shells. See trixi-framework/TrixiAtmo.jl#25.

earth.mp4

@amrueda amrueda marked this pull request as draft November 21, 2023 14:59
@amrueda amrueda changed the title Added the posibility to plot 2D surfaces in 3D WIP: Added the posibility to plot 2D surfaces in 3D Nov 21, 2023
@amrueda amrueda changed the title WIP: Added the posibility to plot 2D surfaces in 3D WIP: Add the posibility to plot 2D surfaces in 3D Nov 21, 2023
@tristanmontoya
Copy link
Member

@amrueda is this ready to merge? It would be nice to have this so we can use it with TrixiAtmo.jl.

@amrueda amrueda marked this pull request as ready for review August 15, 2024 12:04
@amrueda amrueda changed the title WIP: Add the posibility to plot 2D surfaces in 3D Add the posibility to plot 2D surfaces in 3D Aug 15, 2024
@tristanmontoya tristanmontoya removed the request for review from andrewwinters5000 September 7, 2024 10:50
@tristanmontoya
Copy link
Member

tristanmontoya commented Sep 7, 2024

Since Trixi2Vtk.jl calls Trixi.calc_node_coordinates!, plotting 2D surfaces in 3D will not work until Trixi.calc_node_coordinates! can handle this case. I currently have a PR for Trixi.jl at trixi-framework/Trixi.jl#2068 which addresses this for P4estMesh. No changes would need to be made to this PR for Trixi2Vtk.jl if that is merged.

@amrueda
Copy link
Author

amrueda commented Sep 17, 2024

Since Trixi2Vtk.jl calls Trixi.calc_node_coordinates!, plotting 2D surfaces in 3D will not work until Trixi.calc_node_coordinates! can handle this case. I currently have a PR for Trixi.jl at trixi-framework/Trixi.jl#2068 which addresses this for P4estMesh. No changes would need to be made to this PR for Trixi2Vtk.jl if that is merged.

Since trixi-framework/Trixi.jl#2068 was merged, I guess that we are ready to merge this PR?

@@ -234,8 +234,9 @@ end
function calc_node_coordinates(mesh::P4estMesh, nodes, n_visnodes)
# Extract number of spatial dimensions
ndims_ = ndims(mesh)
ndims_spa = size(mesh.tree_node_coordinates,1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment explaining why we need this convoluted way and cannot just use ndims_.

Also, please be consistent with our formatting conventions (space after comma).

Finally, please do not use abbreviations in variable/type/function names that are not absolutely clear to everyone in the community (unless you are really referring to the town in Belgium).

Copy link
Member

@tristanmontoya tristanmontoya Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that trixi-framework/Trixi.jl#2068 is merged, instead of doing this, we can just extract NDIMS and NDIMS_AMBIENT (this is the same as the variable ndims_spa here) from the type parameters of the mesh.

@@ -580,7 +581,7 @@ function calc_vtk_points_cells(node_coordinates::AbstractArray{<:Any,4})
linear_indices = LinearIndices(size_[2:end])

# Use lagrange nodes as VTK points
vtk_points = reshape(node_coordinates, (2, n_points))
vtk_points = reshape(node_coordinates, (size(node_coordinates, 1), n_points))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As before, please leave a short comment explaining this seemingly overcomplex way of setting the number of dimensions.

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.

3 participants