Use enum instead of symbols for indexing #2178
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||
| using OrdinaryDiffEq | ||||||
| using Trixi | ||||||
| import Trixi.Indexing | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
|
|
||||||
| ############################################################################### | ||||||
| # Two semidiscretizations of the ideal GLM-MHD systems using converter functions such that | ||||||
|
|
@@ -49,9 +50,13 @@ mesh1 = StructuredMesh(cells_per_dimension, | |||||
| periodicity = (false, true)) | ||||||
|
|
||||||
| coupling_function1 = (x, u, equations_other, equations_own) -> u | ||||||
| boundary_conditions1 = (x_neg = BoundaryConditionCoupled(2, | ||||||
| (Indexing.last, | ||||||
| Indexing.i_forward), Float64, | ||||||
| coupling_function1), | ||||||
| x_pos = BoundaryConditionCoupled(2, | ||||||
| (Indexing.first, | ||||||
| Indexing.i_forward), Float64, | ||||||
| coupling_function1), | ||||||
| y_neg = boundary_condition_periodic, | ||||||
| y_pos = boundary_condition_periodic) | ||||||
|
|
@@ -72,9 +77,13 @@ mesh2 = StructuredMesh(cells_per_dimension, | |||||
| periodicity = (false, true)) | ||||||
|
|
||||||
| coupling_function2 = (x, u, equations_other, equations_own) -> u | ||||||
| boundary_conditions2 = (x_neg = BoundaryConditionCoupled(1, | ||||||
| (Indexing.last, | ||||||
| Indexing.i_forward), Float64, | ||||||
| coupling_function2), | ||||||
| x_pos = BoundaryConditionCoupled(1, | ||||||
| (Indexing.first, | ||||||
| Indexing.i_forward), Float64, | ||||||
| coupling_function2), | ||||||
| y_neg = boundary_condition_periodic, | ||||||
| y_pos = boundary_condition_periodic) | ||||||
|
|
||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 (Indexing.first, Indexing.last) | ||
| other_orientation = 1 | ||
| elseif indices[2] in (Indexing.first, Indexing.last) | ||
| other_orientation = 2 | ||
| else # indices[3] in (Indexing.first, Indexing.last) | ||
| other_orientation = 3 | ||
| end | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -751,6 +751,32 @@ function compute_coefficients!(u, func, t, mesh::AbstractMesh{3}, equations, dg: | |
| end | ||
| end | ||
| end | ||
| 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
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 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, | ||
|
|
@@ -765,4 +791,3 @@ include("dgsem_structured/dg.jl") | |
| include("dgsem_unstructured/dg.jl") | ||
| include("dgsem_p4est/dg.jl") | ||
| include("dgsem_t8code/dg.jl") | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, IndexInfo}} # [primary/secondary, interface] | ||
|
Comment on lines
132
to
+134
There was a problem hiding this comment. @sloede will tell you that you should consider using a consistent alignment of the comments here and in the other files below |
||
|
|
||
| # internal `resize!`able storage | ||
| _u::Vector{uEltype} | ||
| _neighbor_ids::Vector{Int} | ||
| _node_indices::Vector{NTuple{NDIMS, IndexInfo}} | ||
| end | ||
|
|
||
| @inline function ninterfaces(interfaces::P4estInterfaceContainer) | ||
|
|
@@ -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, 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, | ||
|
|
@@ -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, IndexInfo}} # [boundary] | ||
| name::Vector{Symbol} # [boundary] | ||
|
|
||
| # internal `resize!`able storage | ||
|
|
@@ -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, IndexInfo}}(undef, n_boundaries) | ||
| names = Vector{Symbol}(undef, n_boundaries) | ||
|
|
||
| boundaries = P4estBoundaryContainer{NDIMS, uEltype, NDIMS + 1}(u, neighbor_ids, | ||
|
|
@@ -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, IndexInfo}} # [small/large, mortar] | ||
|
|
||
| # internal `resize!`able storage | ||
| _u::Vector{uEltype} | ||
| _neighbor_ids::Vector{Int} | ||
| _node_indices::Vector{NTuple{NDIMS, IndexInfo}} | ||
| end | ||
|
|
||
| @inline nmortars(mortars::P4estMortarContainer) = size(mortars.neighbor_ids, 2) | ||
|
|
@@ -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, 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, | ||
|
|
@@ -706,17 +706,17 @@ end | |
|
|
||
| # Return direction of the face, which is indexed by node_indices | ||
| @inline function indices2direction(indices) | ||
| if indices[1] === Indexing.first | ||
| return 1 | ||
| elseif indices[1] === Indexing.last | ||
| return 2 | ||
| elseif indices[2] === Indexing.first | ||
| return 3 | ||
| elseif indices[2] === Indexing.last | ||
| return 4 | ||
| elseif indices[3] === Indexing.first | ||
| return 5 | ||
| else # if indices[3] === Indexing.last | ||
| return 6 | ||
| end | ||
| end | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If the coupling stuff becomes non-experimental, we should export this from Trixi.jl.
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.
What not
Not a big fan of
importand it seems like the Julia community is slowly eschewingimport. But it's not a big deal either IMHO, so we can also just leave it as it is.