Reduce number of submodules #60

Closed ranocha closed 4 years ago

In GitLab by @sloede on May 19, 2020, 12:37

As suggested at least once by @ranocha, reducing the number of submodules has some benefits. I agree with him, but I also see some downsides in doing that.

Pro (reduction of submodule count):

Simplifies using Trixi from the REPL
Simplifies the use of @autodocs in the documentation

Con:

Harder to decide where a function/method lives "logically"
As long as "submodule structure = file system structure" and us always using explicit using Module: symbol syntax, it is IMHO much easier to reason about where something is implemented and what is executed when.

But I am all for being convinced that my potential downsides are negligible in comparison to the benefits this would bring.

In GitLab by @sloede on May 19, 2020, 12:38

mentioned in merge request !47

In GitLab by @gregorgassner on May 19, 2020, 12:56

what would be an example of a submodule we could get rid of?

In GitLab by @ranocha on May 19, 2020, 12:57

For some performance debugging, it is really nice to be able to run some code in the REPL. For example, I used this approach for #58. Instead of writing calc_volume_integral! as in Trixi itself, I had to type something like Trixi.Solvers.DgSolver.calc_volume_integral! which is much more verbose. Even If we don't export this function (which would be my approach), typing Trixi.calc_volume_integral! is much easier and I don't have to guess which module structure I have to use.
Right now, some functions are defined only in different modules and hence not really reusable/general, e.g. lax_friedrichs_flux:
```
julia> using Trixi

julia> Trixi.Equations.LinearScalarAdvectionEquations.lax_friedrichs_flux == 
Trixi.Equations.EulerEquations.lax_friedrichs_flux
false
```
I would prefer having a single function lax_friedrichs_flux with different methods implemented for each equation. Using the current structure, such an approach is more complicated and requires much more boilerplate code.
We can get rid of things like import .L2Projection # Import to satisfy Gregor :wink:
Harder to decide where a function/method lives "logically"

Right now, I also find that structure not really clear, to be honest. We can keep the definitions where they are now at the moment. If one want to know which function is called and where it lives, one can either use some editor on steroids or @which in the REPL.

In GitLab by @ranocha on May 19, 2020, 12:58

Basically all modules except Trixi. You can just include a file with definitions and code without having to make it a module. Then, everything could live in the module Trixi.

In GitLab by @gregorgassner on May 19, 2020, 15:50

Sounds interesting to me and I am in general open for it.

But how would we then organize the code parts and files? Does every function then get its own file?

In GitLab by @ranocha on May 19, 2020, 15:55

But how would we then organize the code parts and files? Does every function then get its own file?

No, definitely not. We're luckily not writing MATLAB code. We can basically leave everything as it is but just remove the module Whatever ... end parts.

In GitLab by @andrewwinters5000 on May 19, 2020, 15:58

Interesting, this style of function writing and inclusion is like old school Fortran ;)

In GitLab by @gregorgassner on May 19, 2020, 17:05

I am in general ok with it. What we lose is that we don't see where this function is coming from, right? You see that a function is used somewhere in the file...but you have no idea, where this is from and where to go next if you want to have a detailed look??

So how could we include this information? as a comment when using the include? like having include and then stated in a comment what we actually use from it?

In GitLab by @ranocha on May 19, 2020, 17:55

Usually in Julia packages, only the central file ProjectName.jl includes all relevant files and defines the exports, e.g. https://github.com/SciML/OrdinaryDiffEq.jl/blob/master/src/OrdinaryDiffEq.jl. So I would try to reduce the number of include statements and just gather everything in Trixi.jl if possible.

If one wants to know where a certain function is defined and what it does, one can

search the code in any editor
use an editor that helps with that
use the REPL and @which

use the REPL and foo( + tab, which yields something like

julia> using Trixi

julia> Trixi.Solvers.DgSolver.calc_volume_integral!(
calc_volume_integral!(dg) in Trixi.Solvers.DgSolver at /.../.julia/dev/Trixi/src/solvers/dg.jl:832
calc_volume_integral!(dg, ::Val{:weak_form}, u_t::Array{Float64,4},  dhat::StaticArrays.SArray{Tuple{S1,S2},T,2,L} where L where T where S2 where S1) in Trixi.Solvers.DgSolver at /.../.julia/dev/Trixi/src/solvers/dg.jl:847
calc_volume_integral!(dg, ::Val{:split_form}, u_t::Array{Float64,4}, dsplit_transposed::StaticArrays.SArray{Tuple{S1,S2},T,2,L} where L where T where S2 where S1) in Trixi.Solvers.DgSolver at /.../.julia/dev/Trixi/src/solvers/dg.jl:880
calc_volume_integral!(dg, ::Val{:shock_capturing}, u_t::Array{Float64,4}, dsplit_transposed::StaticArrays.SArray{Tuple{S1,S2},T,2,L} where L where T where S2 where S1) in Trixi.Solvers.DgSolver at /.../.julia/dev/Trixi/src/solvers/dg.jl:920
calc_volume_integral!(dg, ::Val{:shock_capturing}, u_t::Array{Float64,4}, dsplit_transposed::StaticArrays.SArray{Tuple{S1,S2},T,2,L} where L where T where S2 where S1,  inverse_weights::StaticArrays.SArray{Tuple{S},T,1,S} where T where S, alpha::Array{Float64,1}) in Trixi.Solvers.DgSolver at /.../.julia/dev/Trixi/src/solvers/dg.jl:935

In GitLab by @sloede on May 20, 2020, 08:00

In general I am open for this as well. The one relevant disadvantage I still see is that "static, human-based lookup" (i.e., just by looking at the code in a single file, I have a pretty good idea which function is called and where it is defined) is not going to work as easily anymore. But the last trick you mentioned (i.e., being able to use tab completion in the REPL to find all methods) can alleviate this issue.

But to understand it a little better - where, in your opinion @ranocha, would it still make sense to keep a submodule? For example, the Containers submodule and the Trees submodule are very self-contained and re-usable outside Trixi itself (if someone were interested). Would it make sense to keep those as submodules, or would you just get rid of all submodules? Or, to be more general, what is a good reason for introducing a submodule?

In GitLab by @ranocha on May 20, 2020, 08:18

In general, I'm okay with having submodules if they can be used on their own (and are not deeply nested). Basically, my line of reasoning is "If it can be a useful package on its own, it's okay to have it as a submodule (but it should better be a package on its own)."

If module Trees and module Containers provide some useful functionality and do not depend on the rest of Trixi, keeping them as submodules seems to be okay for me. If something depends a lot on other stuff in Trixi and is of no use without the rest, that's a sign that it shouldn't be a submodule, I think.

In GitLab by @sloede on May 25, 2020, 08:12

OK, then let's keep module Trees and module Containers and get rid of all other submodules. However, when doing so we should also review in what we export in the removed submodules: I think some exports were not really meant to be exports from Trixi itself, but only from within the respective submodules. I don't think it's a particularly important problem right now, but we should keep it in mind when we do the actual removal.

In GitLab by @gregorgassner on May 25, 2020, 10:42

I am also open for this change. In general, it seems good to me to gather everything in Trixi main file.

It will just take me some time to adjust and use the other tools to find where the function is coming from...that is not a big deal.

In GitLab by @ranocha on May 26, 2020, 16:25

mentioned in commit abb41ae6cf2b7a8a73f07de11e82ac9b2cc5b7c4

In GitLab by @ranocha on May 27, 2020, 07:05

mentioned in commit 45f2c0c0b664b72b32567639efae1c1ac7a1244f

In GitLab by @sloede on May 27, 2020, 09:41

closed via merge request !54

In GitLab by @sloede on May 27, 2020, 09:41

mentioned in commit 16f32abf20a18cb66e1edf6d8e9f9309701f4931

trixi-framework / Trixi.jl

Reduce number of submodules #60