TrixiParticles.jl icon indicating copy to clipboard operation
TrixiParticles.jl copied to clipboard

Move fields that are only sometimes needed to the cache?

Open efaulhaber opened this issue 1 year ago • 8 comments

Okay, how about this? We reduce fields to the minimum, and everything that is only needed when some method is used goes to the cache. Then we define

@inline function color(system)
    @assert haskey(system.cache, :color) "`color` of a `$(nameof(typeof(system)))` accessed, but there is no color method defined for this system"
    return system.cache.color
end

We could do the same for example with buffer, which is only needed by open boundaries and set to nothing in most simulations. I think this could make the system structs more readable. What do you think, @LasNikas?

Originally posted by @efaulhaber in https://github.com/trixi-framework/TrixiParticles.jl/pull/539#discussion_r1856702070

efaulhaber avatar Nov 25 '24 14:11 efaulhaber

Yes, that's a good idea. Also, the constructor of the systems is starting to grow with argument error checking and creating caches etc. Should we discuss a solution for this as well? I was thinking of something like in Semidiscretization with the check_configuration. It's clear and doesn't mess the constructor.

LasNikas avatar Nov 25 '24 14:11 LasNikas

Yes, check_configuration is a good idea for the systems as well. I suggested earlier that we add a kwarg techniques or so to be used as follows.

WeaklyCompressibleSPHSystem(..., techniques=(DensityDiffusionMolteniColagrossi(delta=0.1),
                                             KernelGradientCorrection(),
                                             TransportVelocityFormulation())

Most of our current kwargs are optional techniques to improve simulations.

efaulhaber avatar Nov 26 '24 09:11 efaulhaber

One downside is that it is not clear which system supports which fields. So this seems to me more a thing that should be done maybe in a 0.3 refactor?

svchb avatar Dec 02 '24 11:12 svchb

Well, it should come with a nice table in the docs listing all available techniques vs all available systems, and then showing ✅ or ❌ for each combination, similar to the README of PointNeighbors..

efaulhaber avatar Dec 02 '24 11:12 efaulhaber

You can even make this type stable if you want, by adding type-based traits. Similar to what we have done with have_nonconservative_terms in Trixi.jl.

That's an optimization though and should only be used when type stability is (can be) an issue.

sloede avatar Dec 02 '24 12:12 sloede

Why wouldn't the Tuple be type stable?

efaulhaber avatar Dec 02 '24 14:12 efaulhaber

I'm pretty sure that the @assert does some Weird Stuff under the hood. At least the code path for a system not containing color I suspect of being not type stable. But it also depends on how you use it - if it is strictly for catching errors then it might be ok

sloede avatar Dec 02 '24 22:12 sloede

Ah you were referring to the code in the first comment. I checked the LLVM when passing a tuple, and the check is indeed optimized away.

efaulhaber avatar Dec 02 '24 22:12 efaulhaber