Use enum instead of symbols for indexing #2178

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

Review checklist

This checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging.

Purpose and scope

The PR has a single goal that is clear from the PR title and/or description.
All code changes represent a single set of modifications that logically belong together.
No more than 500 lines of code are changed or there is no obvious way to split the PR into multiple PRs.

Code quality

The code can be understood easily.
Newly introduced names for variables etc. are self-descriptive and consistent with existing naming conventions.
There are no redundancies that can be removed by simple modularization/refactoring.
There are no leftover debug statements or commented code sections.
The code adheres to our conventions and style guide, and to the Julia guidelines.

Documentation

New functions and types are documented with a docstring or top-level comment.
Relevant publications are referenced in docstrings (see example for formatting).
Inline comments are used to document longer or unusual code sections.
Comments describe intent ("why?") and not just functionality ("what?").
If the PR introduces a significant change or new feature, it is documented in NEWS.md with its PR number.

Testing

The PR passes all tests.
New or modified lines of code are covered by tests.
New or modified tests run in less then 10 seconds.

Performance

There are no type instabilities or memory allocations in performance-critical parts.
If the PR intent is to improve performance, before/after time measurements are posted in the PR.

Verification

The correctness of the code was verified using appropriate tests.
If new equations/methods are added, a convergence test has been run and the results
are posted in the PR.

Created with ❤️ by the Trixi.jl community.

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

Codecov Report

Attention: 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 1 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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use enum instead of symbols for indexing #2178

Use enum instead of symbols for indexing #2178

benegee commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024

This comment was marked as outdated.

codecov bot commented Nov 25, 2024 •

edited

Loading

benegee commented Nov 25, 2024

sloede commented Nov 27, 2024

ranocha left a comment

ranocha Nov 27, 2024 •

edited

Loading

sloede Nov 28, 2024

ranocha Nov 27, 2024 •

edited

Loading

ranocha Nov 27, 2024 •

edited

Loading

ranocha Nov 27, 2024

sloede Nov 28, 2024

ranocha Nov 27, 2024

sloede Nov 28, 2024

vchuravy commented Nov 27, 2024

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

Use enum instead of symbols for indexing #2178

Are you sure you want to change the base?

Use enum instead of symbols for indexing #2178

Conversation

benegee commented Nov 25, 2024 • edited Loading

github-actions bot commented Nov 25, 2024

Review checklist

Purpose and scope

Code quality

Documentation

Testing

Performance

Verification

This comment was marked as outdated.

codecov bot commented Nov 25, 2024 • edited Loading

Codecov Report

benegee commented Nov 25, 2024

sloede commented Nov 27, 2024

ranocha left a comment

Choose a reason for hiding this comment

ranocha Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

sloede Nov 28, 2024

Choose a reason for hiding this comment

ranocha Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

ranocha Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

ranocha Nov 27, 2024

Choose a reason for hiding this comment

sloede Nov 28, 2024

Choose a reason for hiding this comment

ranocha Nov 27, 2024

Choose a reason for hiding this comment

sloede Nov 28, 2024

Choose a reason for hiding this comment

vchuravy commented Nov 27, 2024

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