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

Use enum instead of symbols for indexing #2178

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 17 additions & 16 deletions examples/structured_2d_dgsem/elixir_advection_coupled.jl
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using OrdinaryDiffEq
using Trixi
import Trixi.Indexing
Copy link
Member

@ranocha ranocha Nov 27, 2024

Choose a reason for hiding this comment

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

If the coupling stuff becomes non-experimental, we should export this from Trixi.jl.

Suggested change
import Trixi.Indexing
using Trixi: Indexing # coupling is an experimental feature

Copy link
Member

Choose a reason for hiding this comment

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

What not

Suggested change
import Trixi.Indexing
using Trixi: Indexing

Not a big fan of import and it seems like the Julia community is slowly eschewing import. But it's not a big deal either IMHO, so we can also just leave it as it is.


###############################################################################
# Coupled semidiscretization of four linear advection systems using converter functions such that
Expand Down Expand Up @@ -61,13 +62,13 @@ coupling_function12 = (x, u, equations_other, equations_own) -> u
coupling_function13 = (x, u, equations_other, equations_own) -> u

# Define the coupling boundary conditions and the system it is coupled to.
boundary_conditions_x_neg1 = BoundaryConditionCoupled(2, (:end, :i_forward), Float64,
boundary_conditions_x_neg1 = BoundaryConditionCoupled(2, (Indexing.last, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function12)
boundary_conditions_x_pos1 = BoundaryConditionCoupled(2, (:begin, :i_forward), Float64,
boundary_conditions_x_pos1 = BoundaryConditionCoupled(2, (Indexing.first, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function12)
boundary_conditions_y_neg1 = BoundaryConditionCoupled(3, (:i_forward, :end), Float64,
boundary_conditions_y_neg1 = BoundaryConditionCoupled(3, (Indexing.i_forward, Indexing.last), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function13)
boundary_conditions_y_pos1 = BoundaryConditionCoupled(3, (:i_forward, :begin), Float64,
boundary_conditions_y_pos1 = BoundaryConditionCoupled(3, (Indexing.i_forward, Indexing.first), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function13)

# A semidiscretization collects data structures and functions for the spatial discretization
Expand All @@ -93,13 +94,13 @@ coupling_function21 = (x, u, equations_other, equations_own) -> u
coupling_function24 = (x, u, equations_other, equations_own) -> u

# Define the coupling boundary conditions and the system it is coupled to.
boundary_conditions_x_neg2 = BoundaryConditionCoupled(1, (:end, :i_forward), Float64,
boundary_conditions_x_neg2 = BoundaryConditionCoupled(1, (Indexing.last, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function21)
boundary_conditions_x_pos2 = BoundaryConditionCoupled(1, (:begin, :i_forward), Float64,
boundary_conditions_x_pos2 = BoundaryConditionCoupled(1, (Indexing.first, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function21)
boundary_conditions_y_neg2 = BoundaryConditionCoupled(4, (:i_forward, :end), Float64,
boundary_conditions_y_neg2 = BoundaryConditionCoupled(4, (Indexing.i_forward, Indexing.last), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function24)
boundary_conditions_y_pos2 = BoundaryConditionCoupled(4, (:i_forward, :begin), Float64,
boundary_conditions_y_pos2 = BoundaryConditionCoupled(4, (Indexing.i_forward, Indexing.first), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function24)

# A semidiscretization collects data structures and functions for the spatial discretization
Expand All @@ -125,13 +126,13 @@ coupling_function34 = (x, u, equations_other, equations_own) -> u
coupling_function31 = (x, u, equations_other, equations_own) -> u

# Define the coupling boundary conditions and the system it is coupled to.
boundary_conditions_x_neg3 = BoundaryConditionCoupled(4, (:end, :i_forward), Float64,
boundary_conditions_x_neg3 = BoundaryConditionCoupled(4, (Indexing.last, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function34)
boundary_conditions_x_pos3 = BoundaryConditionCoupled(4, (:begin, :i_forward), Float64,
boundary_conditions_x_pos3 = BoundaryConditionCoupled(4, (Indexing.first, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function34)
boundary_conditions_y_neg3 = BoundaryConditionCoupled(1, (:i_forward, :end), Float64,
boundary_conditions_y_neg3 = BoundaryConditionCoupled(1, (Indexing.i_forward, Indexing.last), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function31)
boundary_conditions_y_pos3 = BoundaryConditionCoupled(1, (:i_forward, :begin), Float64,
boundary_conditions_y_pos3 = BoundaryConditionCoupled(1, (Indexing.i_forward, Indexing.first), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function31)

# A semidiscretization collects data structures and functions for the spatial discretization
Expand All @@ -157,13 +158,13 @@ coupling_function43 = (x, u, equations_other, equations_own) -> u
coupling_function42 = (x, u, equations_other, equations_own) -> u

# Define the coupling boundary conditions and the system it is coupled to.
boundary_conditions_x_neg4 = BoundaryConditionCoupled(3, (:end, :i_forward), Float64,
boundary_conditions_x_neg4 = BoundaryConditionCoupled(3, (Indexing.last, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function43)
boundary_conditions_x_pos4 = BoundaryConditionCoupled(3, (:begin, :i_forward), Float64,
boundary_conditions_x_pos4 = BoundaryConditionCoupled(3, (Indexing.first, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function43)
boundary_conditions_y_neg4 = BoundaryConditionCoupled(2, (:i_forward, :end), Float64,
boundary_conditions_y_neg4 = BoundaryConditionCoupled(2, (Indexing.i_forward, Indexing.last), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function42)
boundary_conditions_y_pos4 = BoundaryConditionCoupled(2, (:i_forward, :begin), Float64,
boundary_conditions_y_pos4 = BoundaryConditionCoupled(2, (Indexing.i_forward, Indexing.first), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function42)

# A semidiscretization collects data structures and functions for the spatial discretization
Expand Down
10 changes: 5 additions & 5 deletions examples/structured_2d_dgsem/elixir_advection_meshview.jl
Original file line number Diff line number Diff line change
Expand Up @@ -51,21 +51,21 @@ mesh2 = StructuredMeshView(parent_mesh; indices_min = (9, 1), indices_max = (16,
coupling_function = (x, u, equations_other, equations_own) -> u

# Define the coupled boundary conditions
# The indices (:end, :i_forward) and (:begin, :i_forward) denote the interface indexing.
# The indices (Indexing.last, Indexing.i_forward) and (Indexing.first, Indexing.i_forward) denote the interface indexing.
# For a system with coupling in x and y see examples/structured_2d_dgsem/elixir_advection_coupled.jl.
boundary_conditions1 = (
# Connect left boundary with right boundary of left mesh
x_neg = BoundaryConditionCoupled(2, (:end, :i_forward), Float64,
x_neg = BoundaryConditionCoupled(2, (Indexing.last, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function),
x_pos = BoundaryConditionCoupled(2, (:begin, :i_forward), Float64,
x_pos = BoundaryConditionCoupled(2, (Indexing.first, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function),
y_neg = boundary_condition_periodic,
y_pos = boundary_condition_periodic)
boundary_conditions2 = (
# Connect left boundary with right boundary of left mesh
x_neg = BoundaryConditionCoupled(1, (:end, :i_forward), Float64,
x_neg = BoundaryConditionCoupled(1, (Indexing.last, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function),
x_pos = BoundaryConditionCoupled(1, (:begin, :i_forward), Float64,
x_pos = BoundaryConditionCoupled(1, (Indexing.first, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function),
y_neg = boundary_condition_periodic,
y_pos = boundary_condition_periodic)
Expand Down
8 changes: 4 additions & 4 deletions examples/structured_2d_dgsem/elixir_mhd_coupled.jl
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ mesh1 = StructuredMesh(cells_per_dimension,
periodicity = (false, true))

coupling_function1 = (x, u, equations_other, equations_own) -> u
boundary_conditions1 = (x_neg = BoundaryConditionCoupled(2, (:end, :i_forward), Float64,
boundary_conditions1 = (x_neg = BoundaryConditionCoupled(2, (Indexing.last, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function1),
x_pos = BoundaryConditionCoupled(2, (:begin, :i_forward), Float64,
x_pos = BoundaryConditionCoupled(2, (Indexing.first, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function1),
y_neg = boundary_condition_periodic,
y_pos = boundary_condition_periodic)
Expand All @@ -72,9 +72,9 @@ mesh2 = StructuredMesh(cells_per_dimension,
periodicity = (false, true))

coupling_function2 = (x, u, equations_other, equations_own) -> u
boundary_conditions2 = (x_neg = BoundaryConditionCoupled(1, (:end, :i_forward), Float64,
boundary_conditions2 = (x_neg = BoundaryConditionCoupled(1, (Indexing.last, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function2),
x_pos = BoundaryConditionCoupled(1, (:begin, :i_forward), Float64,
x_pos = BoundaryConditionCoupled(1, (Indexing.first, Indexing.i_forward), Float64,
benegee marked this conversation as resolved.
Show resolved Hide resolved
coupling_function2),
y_neg = boundary_condition_periodic,
y_pos = boundary_condition_periodic)
Expand Down
6 changes: 3 additions & 3 deletions src/semidiscretization/semidiscretization_coupled.jl
Original file line number Diff line number Diff line change
Expand Up @@ -452,11 +452,11 @@ mutable struct BoundaryConditionCoupled{NDIMS,
NDIMS = length(indices)
u_boundary = Array{uEltype, NDIMS * 2 - 1}(undef, ntuple(_ -> 0, NDIMS * 2 - 1))

if indices[1] in (:begin, :end)
if indices[1] in (Indexing.first, Indexing.last)
other_orientation = 1
elseif indices[2] in (:begin, :end)
elseif indices[2] in (Indexing.first, Indexing.last)
other_orientation = 2
else # indices[3] in (:begin, :end)
else # indices[3] in (Indexing.first, Indexing.last)
other_orientation = 3
end

Expand Down
28 changes: 27 additions & 1 deletion src/solvers/dg.jl
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,33 @@ function compute_coefficients!(u, func, t, mesh::AbstractMesh{3}, equations, dg:
end
end

benegee marked this conversation as resolved.
Show resolved Hide resolved
end # @muladd; put it up here because module definition below needs to be at top level

# For some mesh types, elements next to a surface may have local coordinate systems
# that are not aligned so the nodes may have to be indexed differently.
# `IndexInfo` is used to describe how the nodes should be indexed.
# For example, in 2d a `Tuple` with two `IndexInfo` objects, one for each dimension,
# would be used.
# `first` or `last` indicates that the corresponding index is constant and is either
# the first or the last one. This effectively encodes the position of the surface
# with respect to the local coordinate system. The other `IndexInfo` object(s)
# encode if the index in the corresponding dimension is running forward or backward.
#
# The Enum is wrapped in a module and exported so that the enum values do not pollute
# the global namespace and can only be accessed via `Indexing.value`.
module Indexing
@enum IndexInfo begin
first
last
i_forward
i_backward
j_forward
j_backward
end
export IndexInfo
end
using .Indexing
Comment on lines +754 to +779
Copy link
Member

Choose a reason for hiding this comment

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

This is basically https://github.com/fredrikekre/EnumX.jl, so I think we should use that package instead. We should also discuss where to move this definition - https://github.com/trixi-framework/Trixi.jl/blob/main/src/basic_types.jl or auxiliary could also make sense.

Copy link
Member

Choose a reason for hiding this comment

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

I tend to agree to use specialized packages wherever sensible. However, does it really improve anything except reducing our code by ~10 lines, at the cost of (ideally) having a comment explain what the @enumx does?

But I'm fine either way 🤷‍♂️


# Discretizations specific to each mesh type of Trixi.jl
# If some functionality is shared by multiple combinations of meshes/solvers,
# it is defined in the directory of the most basic mesh and solver type.
Expand All @@ -765,4 +792,3 @@ include("dgsem_structured/dg.jl")
include("dgsem_unstructured/dg.jl")
include("dgsem_p4est/dg.jl")
include("dgsem_t8code/dg.jl")
end # @muladd
28 changes: 14 additions & 14 deletions src/solvers/dgsem_p4est/containers.jl
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,12 @@ mutable struct P4estInterfaceContainer{NDIMS, uEltype <: Real, NDIMSP2} <:
AbstractContainer
u::Array{uEltype, NDIMSP2} # [primary/secondary, variable, i, j, interface]
neighbor_ids::Matrix{Int} # [primary/secondary, interface]
node_indices::Matrix{NTuple{NDIMS, Symbol}} # [primary/secondary, interface]
node_indices::Matrix{NTuple{NDIMS, IndexInfo}} # [primary/secondary, interface]
Comment on lines 132 to +134
Copy link
Member

Choose a reason for hiding this comment

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

@sloede will tell you that you should consider using a consistent alignment of the comments here and in the other files below

Copy link
Member

Choose a reason for hiding this comment

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

That would be great indeed 😍


# internal `resize!`able storage
_u::Vector{uEltype}
_neighbor_ids::Vector{Int}
_node_indices::Vector{NTuple{NDIMS, Symbol}}
_node_indices::Vector{NTuple{NDIMS, IndexInfo}}
end

@inline function ninterfaces(interfaces::P4estInterfaceContainer)
Expand Down Expand Up @@ -184,7 +184,7 @@ function init_interfaces(mesh::Union{P4estMesh, T8codeMesh}, equations, basis, e
_neighbor_ids = Vector{Int}(undef, 2 * n_interfaces)
neighbor_ids = unsafe_wrap(Array, pointer(_neighbor_ids), (2, n_interfaces))

_node_indices = Vector{NTuple{NDIMS, Symbol}}(undef, 2 * n_interfaces)
_node_indices = Vector{NTuple{NDIMS, IndexInfo}}(undef, 2 * n_interfaces)
node_indices = unsafe_wrap(Array, pointer(_node_indices), (2, n_interfaces))

interfaces = P4estInterfaceContainer{NDIMS, uEltype, NDIMS + 2}(u, neighbor_ids,
Expand All @@ -207,7 +207,7 @@ mutable struct P4estBoundaryContainer{NDIMS, uEltype <: Real, NDIMSP1} <:
AbstractContainer
u::Array{uEltype, NDIMSP1} # [variables, i, j, boundary]
neighbor_ids::Vector{Int} # [boundary]
node_indices::Vector{NTuple{NDIMS, Symbol}} # [boundary]
node_indices::Vector{NTuple{NDIMS, IndexInfo}} # [boundary]
name::Vector{Symbol} # [boundary]

# internal `resize!`able storage
Expand Down Expand Up @@ -257,7 +257,7 @@ function init_boundaries(mesh::Union{P4estMesh, T8codeMesh}, equations, basis, e
n_boundaries))

neighbor_ids = Vector{Int}(undef, n_boundaries)
node_indices = Vector{NTuple{NDIMS, Symbol}}(undef, n_boundaries)
node_indices = Vector{NTuple{NDIMS, IndexInfo}}(undef, n_boundaries)
names = Vector{Symbol}(undef, n_boundaries)

boundaries = P4estBoundaryContainer{NDIMS, uEltype, NDIMS + 1}(u, neighbor_ids,
Expand Down Expand Up @@ -338,12 +338,12 @@ mutable struct P4estMortarContainer{NDIMS, uEltype <: Real, NDIMSP1, NDIMSP3} <:
AbstractContainer
u::Array{uEltype, NDIMSP3} # [small/large side, variable, position, i, j, mortar]
neighbor_ids::Matrix{Int} # [position, mortar]
node_indices::Matrix{NTuple{NDIMS, Symbol}} # [small/large, mortar]
node_indices::Matrix{NTuple{NDIMS, IndexInfo}} # [small/large, mortar]

# internal `resize!`able storage
_u::Vector{uEltype}
_neighbor_ids::Vector{Int}
_node_indices::Vector{NTuple{NDIMS, Symbol}}
_node_indices::Vector{NTuple{NDIMS, IndexInfo}}
end

@inline nmortars(mortars::P4estMortarContainer) = size(mortars.neighbor_ids, 2)
Expand Down Expand Up @@ -391,7 +391,7 @@ function init_mortars(mesh::Union{P4estMesh, T8codeMesh}, equations, basis, elem
neighbor_ids = unsafe_wrap(Array, pointer(_neighbor_ids),
(2^(NDIMS - 1) + 1, n_mortars))

_node_indices = Vector{NTuple{NDIMS, Symbol}}(undef, 2 * n_mortars)
_node_indices = Vector{NTuple{NDIMS, IndexInfo}}(undef, 2 * n_mortars)
node_indices = unsafe_wrap(Array, pointer(_node_indices), (2, n_mortars))

mortars = P4estMortarContainer{NDIMS, uEltype, NDIMS + 1, NDIMS + 3}(u,
Expand Down Expand Up @@ -706,17 +706,17 @@ end

# Return direction of the face, which is indexed by node_indices
@inline function indices2direction(indices)
if indices[1] === :begin
if indices[1] === Indexing.first
return 1
elseif indices[1] === :end
elseif indices[1] === Indexing.last
return 2
elseif indices[2] === :begin
elseif indices[2] === Indexing.first
return 3
elseif indices[2] === :end
elseif indices[2] === Indexing.last
return 4
elseif indices[3] === :begin
elseif indices[3] === Indexing.first
return 5
else # if indices[3] === :end
else # if indices[3] === Indexing.last
return 6
end
end
Expand Down
32 changes: 16 additions & 16 deletions src/solvers/dgsem_p4est/containers_2d.jl
Original file line number Diff line number Diff line change
Expand Up @@ -95,24 +95,24 @@ end
# relative to the interface.
if side == 1 || orientation == 0
# Forward indexing
i = :i_forward
i = Indexing.i_forward
else
# Backward indexing
i = :i_backward
i = Indexing.i_backward
end

if faces[side] == 0
# Index face in negative x-direction
interfaces.node_indices[side, interface_id] = (:begin, i)
interfaces.node_indices[side, interface_id] = (Indexing.first, i)
elseif faces[side] == 1
# Index face in positive x-direction
interfaces.node_indices[side, interface_id] = (:end, i)
interfaces.node_indices[side, interface_id] = (Indexing.last, i)
elseif faces[side] == 2
# Index face in negative y-direction
interfaces.node_indices[side, interface_id] = (i, :begin)
interfaces.node_indices[side, interface_id] = (i, Indexing.first)
else # faces[side] == 3
# Index face in positive y-direction
interfaces.node_indices[side, interface_id] = (i, :end)
interfaces.node_indices[side, interface_id] = (i, Indexing.last)
end
end

Expand All @@ -124,16 +124,16 @@ end
face, boundary_id)
if face == 0
# Index face in negative x-direction
boundaries.node_indices[boundary_id] = (:begin, :i_forward)
boundaries.node_indices[boundary_id] = (Indexing.first, Indexing.i_forward)
elseif face == 1
# Index face in positive x-direction
boundaries.node_indices[boundary_id] = (:end, :i_forward)
boundaries.node_indices[boundary_id] = (Indexing.last, Indexing.i_forward)
elseif face == 2
# Index face in negative y-direction
boundaries.node_indices[boundary_id] = (:i_forward, :begin)
boundaries.node_indices[boundary_id] = (Indexing.i_forward, Indexing.first)
else # face == 3
# Index face in positive y-direction
boundaries.node_indices[boundary_id] = (:i_forward, :end)
boundaries.node_indices[boundary_id] = (Indexing.i_forward, Indexing.last)
end

return boundaries
Expand All @@ -148,24 +148,24 @@ end
# relative to the mortar.
if side == 1 || orientation == 0
# Forward indexing for small side or orientation == 0
i = :i_forward
i = Indexing.i_forward
else
# Backward indexing for large side with reversed orientation
i = :i_backward
i = Indexing.i_backward
end

if faces[side] == 0
# Index face in negative x-direction
mortars.node_indices[side, mortar_id] = (:begin, i)
mortars.node_indices[side, mortar_id] = (Indexing.first, i)
elseif faces[side] == 1
# Index face in positive x-direction
mortars.node_indices[side, mortar_id] = (:end, i)
mortars.node_indices[side, mortar_id] = (Indexing.last, i)
elseif faces[side] == 2
# Index face in negative y-direction
mortars.node_indices[side, mortar_id] = (i, :begin)
mortars.node_indices[side, mortar_id] = (i, Indexing.first)
else # faces[side] == 3
# Index face in positive y-direction
mortars.node_indices[side, mortar_id] = (i, :end)
mortars.node_indices[side, mortar_id] = (i, Indexing.last)
end
end

Expand Down
Loading
Loading