Use enum instead of symbols for indexing by benegee · Pull Request #2178 · trixi-framework/Trixi.jl

benegee · 2024-11-25T12:38:14Z

Some containers in Trixi.jl contain symbols, e.g. node_indices in P4estInterfaceContainer. Symbols are not isbits, which could e.g. prevent GPU offloading.

github-actions · 2024-11-25T12:38:27Z

codecov · 2024-11-25T14:13:45Z

Codecov Report

❌ Patch coverage is 97.97980% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.36%. Comparing base (0046123) to head (033a817).
⚠️ Report is 341 commits behind head on main.

Files with missing lines	Patch %	Lines
src/solvers/dgsem_p4est/containers_3d.jl	94.12%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2178   +/-   ##
=======================================
  Coverage   96.36%   96.36%           
=======================================
  Files         480      480           
  Lines       38230    38230           
=======================================
  Hits        36840    36840           
  Misses       1390     1390

Flag	Coverage Δ
unittests	`96.36% <97.98%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

benegee · 2024-11-25T15:05:52Z

@sloede @ranocha @vchuravy @jmark

Introducing an enum as discussed seems to work.

sloede · 2024-11-27T05:33:42Z

Thanks for taking care of this! Once you're ready, please mark this PR as ready for review and request one from me.

ranocha

Thanks for working on this! Please find below some comments.

ranocha · 2024-11-27T08:35:30Z

examples/structured_2d_dgsem/elixir_advection_coupled.jl

@@ -1,5 +1,6 @@
 using OrdinaryDiffEq
 using Trixi
+import Trixi.Indexing


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

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.

ranocha · 2024-11-27T08:36:03Z

examples/structured_2d_dgsem/elixir_advection_meshview.jl

@@ -1,5 +1,6 @@
 using OrdinaryDiffEq
 using Trixi
+import Trixi.Indexing


Suggested change

import Trixi.Indexing

using Trixi: Indexing # coupling is an experimental feature

ranocha · 2024-11-27T08:36:08Z

examples/structured_2d_dgsem/elixir_mhd_coupled.jl

@@ -1,5 +1,6 @@
 using OrdinaryDiffEq
 using Trixi
+import Trixi.Indexing


Suggested change

import Trixi.Indexing

using Trixi: Indexing # coupling is an experimental feature

ranocha · 2024-11-27T08:42:54Z

src/solvers/dg.jl

+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


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.

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 🤷‍♂️

ranocha · 2024-11-27T08:43:40Z

src/solvers/dgsem_p4est/containers.jl

    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]


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

That would be great indeed 😍

vchuravy · 2024-11-27T15:52:54Z

After speaking with @maleadt in Lausanne and we decided that we might be able to permit Symbols being passed to the GPU JuliaGPU/GPUCompiler.jl#650

change all indexing symbols to enum members

4377d96

This comment was marked as outdated.

Sign in to view

benegee added 4 commits November 25, 2024 14:40

missing import

0c63b8e

missed Symbol

e1d5651

explicitly skip Trixi.Indexing

9b37415

formatter

9a467c9

benegee marked this pull request as ready for review November 27, 2024 05:51

benegee requested a review from sloede November 27, 2024 05:51

Merge branch 'main' into bg/indexing-enum

033a817

ranocha requested changes Nov 27, 2024

View reviewed changes

DanielDoehring added refactoring Refactoring code without functional changes gpu labels Feb 9, 2025

benegee closed this Sep 15, 2025

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

Conversation

benegee commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 25, 2024

Review checklist

Purpose and scope

Code quality

Documentation

Testing

Performance

Verification

Uh oh!

This comment was marked as outdated.

Uh oh!

codecov bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

benegee commented Nov 25, 2024

Uh oh!

sloede commented Nov 27, 2024

Uh oh!

ranocha left a comment

Choose a reason for hiding this comment

Uh oh!

ranocha Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloede Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

ranocha Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranocha Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranocha Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

sloede Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

ranocha Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

sloede Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

vchuravy commented Nov 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benegee commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading

ranocha Nov 27, 2024 •

edited

Loading

ranocha Nov 27, 2024 •

edited

Loading

ranocha Nov 27, 2024 •

edited

Loading