Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change name scalar to density in LinearAdvectionEquation #1910

Open
ArseniyKholod opened this issue Apr 22, 2024 · 4 comments
Open

Change name scalar to density in LinearAdvectionEquation #1910

ArseniyKholod opened this issue Apr 22, 2024 · 4 comments
Labels
consistency Make Michael happy discussion low-priority refactoring Refactoring code without functional changes

Comments

@ArseniyKholod
Copy link
Contributor

Suggestion from the comment.

"The output after line 49 still shows the variable name scalar. I guess this hard-coded in the example.
-> feature request to Trixi.jl: Please, change name scalar to density, because scalar just looks like a type specification :=O"

It's about how name of the variable in LinearScalarAdvection is shown. Take a look for example in
https://github.com/trixi-framework/Trixi.jl/blob/dd051566a9079e22dfa792b145c0b50eaafa37fd/src/equations/linear_scalar_advection_1d.jl#L26C70-L26C76.

The same for 2D and 3D.

@sloede
Copy link
Member

sloede commented Apr 22, 2024

I have no strong feelings here. If it's really only about the scalar advection equation, density is wrong though IMHO. If scalar is confusing, I'd be OK to rename it something neutral like u, but given that this is the first time I hear about it, I'd like to wait a little to see if this proposal gains more support before considering such a change.

@jlchan
Copy link
Contributor

jlchan commented Apr 22, 2024

I like u, though I think concentration would be a better physically-based name than density.

@ArseniyKholod
Copy link
Contributor Author

I have no strong feelings here. If it's really only about the scalar advection equation, density is wrong though IMHO. If scalar is confusing, I'd be OK to rename it something neutral like u, but given that this is the first time I hear about it, I'd like to wait a little to see if this proposal gains more support before considering such a change.

I think this issue could wait, as it's not usage crucial. But may be to ask Manuel @torrilhon, why it should be density?

@torrilhon
Copy link
Contributor

It is REALLY low-priority! My comment really came from somebody (me) who looked at this output literally with zero knowledge about Trixi at all... and I thought "oh, Trixi tells me the type of the advected quantity"

True, 'density' (something per volume) is actually convected differently. I like 'u' because this nicely connects to the documentation!

@DanielDoehring DanielDoehring added discussion refactoring Refactoring code without functional changes labels Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consistency Make Michael happy discussion low-priority refactoring Refactoring code without functional changes
Projects
None yet
Development

No branches or pull requests

5 participants