search

trixi-framework / Trixi.jl

Trixi.jl: Adaptive high-order numerical simulations of conservation laws in Julia
https://trixi-framework.github.io/Trixi.jl
MIT License
524 stars 104 forks source link

Usage of `ndims(mesh)` at many avoidable places #2080

Open DanielDoehring opened 1 week ago

DanielDoehring commented 1 week ago

We often dispatch functions based on the spatial dimensionality of the mesh, see for instance

https://github.com/trixi-framework/Trixi.jl/blob/288d41d61fa357df6fa10dbfe1db514f6be6a59b/src/callbacks_stage/positivity_zhang_shu_dg1d.jl#L8-L9

Nevetheless, in these functions the dimensionality of the mesh is still often queried:

https://github.com/trixi-framework/Trixi.jl/blob/288d41d61fa357df6fa10dbfe1db514f6be6a59b/src/callbacks_stage/positivity_zhang_shu_dg1d.jl#L30

While the overhead for this is hopefully negligible due to optimizations, this is (in my opinion) bad style, as this code suggests some variability in the method which is not there.

ranocha commented 1 week ago

It should be compiled away since it just returns a compile-time constant. In the specific case you mentioned, it makes it much clearer how the magic numbers appear there. For example, it would be hard for me to see how to generalize it if I just saw / 4 in the 2D case.

Having said that, it would be great if you could make some concrete suggestions on how to clarify and improve the code.

DanielDoehring commented 6 days ago

I would suggest to put the dimensionality right there and leave the usage of this function for the places where the mesh is actually generic.

If this leads to confusion, one could think about adding comments (in the example above, there is for instance already a comment explaining the 2.)