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

Convention for the hypoviscosity order: should it be positive or negative #115

Closed
navidcy opened this issue Sep 15, 2020 · 8 comments · Fixed by #174
Closed

Convention for the hypoviscosity order: should it be positive or negative #115

navidcy opened this issue Sep 15, 2020 · 8 comments · Fixed by #174

Comments

@navidcy
Copy link
Member

navidcy commented Sep 15, 2020

At the moment, the code assumes that hypoviscosity order must is negative; see, e.g.,

L = @. - params.ν * grid.Krsq^params.- params.μ * grid.Krsq^params.

but the Docs in some places imply that it is positive, e.g.,
$$\mathcal{L} = -\mu k^{-2n_\mu} - \nu k^{2n_\nu}\ ,$$

Let's choose one of the two conventions and smooth out everything in source code, examples, and docs (across all modules). Opinions?

@glwagner
Copy link
Member

It's not really hypoviscosity but a "second viscosity".

I think it's better to have the primary and secondary viscosity use the same sign convention.

Note that there's nothing that prevents one from using a negative exponent in the primary viscosity, either.

The point of having two viscosities is just flexibility and since we can't dictate that one is "hyper" and one is "hypo", I don't think we should imply anything of the sort. We should point out that this is the intent of this feature, however. Users may also specify Laplacian viscosity and a high-order hyperviscosity.

@navidcy
Copy link
Member Author

navidcy commented Sep 16, 2020

sure... fair point

@BrodiePearson
Copy link
Collaborator

I agree that it should have the same sign convention as the first.

One route in the code is to refer to one as small-scale viscosity and the other as large-scale viscosity by convention. In the Docs it could then be stated that

they are both viscosities and typically one is chosen to act at small scales (n\nu >=1) and the other at large scales (n\mu<=0) with the convention here that \nu acts at small-scales and \mu acts at large-scales. Some common choices are n\nu =1 (molecular-style viscosity) and n\mu=0 (linear drag), while values of n\nu>=2 and m\mu<=-1 are referred to as hyper- and hypo-viscosities respectively.

@navidcy
Copy link
Member Author

navidcy commented Dec 25, 2020

Now I noticed, that with the same sign convention, the energy_drag and energy_dissipation diagnostics are superfluous...

"""
energy_dissipation(prob)
Returns the domain-averaged energy dissipation rate. `nν` must be >= 1.
"""
@inline function energy_dissipation(prob)
sol, vars, params, grid = prob.sol, prob.vars, prob.params, prob.grid
energy_dissipationh = vars.uh # use vars.uh as scratch variable
@. energy_dissipationh = params.ν * grid.Krsq^(params.- 1) * abs2(sol)
CUDA.@allowscalar energy_dissipationh[1, 1] = 0
return 1 / (grid.Lx * grid.Ly) * parsevalsum(energy_dissipationh, grid)
end

"""
energy_drag(prob)
Returns the extraction of domain-averaged energy by drag/hypodrag μ.
"""
@inline function energy_drag(prob)
sol, vars, params, grid = prob.sol, prob.vars, prob.params, prob.grid
energy_dragh = vars.uh # use vars.uh as scratch variable
@. energy_dragh = params.μ * grid.Krsq^(params.- 1) * abs2(sol)
CUDA.@allowscalar energy_dragh[1, 1] = 0
return 1 / (grid.Lx * grid.Ly) * parsevalsum(energy_dragh, grid)
end

They can be combine into a single function with an optional keyword argument? @glwagner any suggestion on what would be the most efficient way to do that? Can we dispatch on the parameter variable name? params.ν vs params.μ?

@BrodiePearson
Copy link
Collaborator

@navidcy You're right about the drag/dissipation being indistinguishable with the same sign convention.

An alternative approach (outlined below) could be to have only one viscous input with two properties (viscosity ν and order ). Since you and @glwagner have more experience with this code maybe you'll know if this is a bad idea or if it would be require significant effort.

  • The two viscous properties could be scalars (e.g. a hyper-viscosity ν=10^5 and order nν=8)
  • Or they could be vectors (e.g. hyper- and hypo-viscosity ν=[10^5, 10^20] and order nν=[8, -2])
  • If they are vectors each element would add another dissipative term to the budget, and there could be one dissipation diagnostic with an extra dimension denoting that viscosity element that produced it.
  • This would be agnostic to the type of dissipation and more generalizable (maybe someone wants 10 different types of viscosity operator acting at once...),
  • But I am not sure whether it would simplify the code (e.g. would the calculation of multiple dissipative budget terms slow the code down, could the multiple dissipation diagnostics be calculated efficiently, and would separate diagnostics/budget functions be required depending on the shape of the inputs?).

@navidcy
Copy link
Member Author

navidcy commented Dec 28, 2020

@BrodiePearson I think you took this one level up :)
I was only referring to the boiler plate for the two diagnostics, energy_drag and energy_dissipation.

I cleared up the boiler-plate with 28a4a76.

@navidcy
Copy link
Member Author

navidcy commented Dec 28, 2020

Your suggestion above is nice but perhaps let's leave it for a future PR and not #174...

@glwagner
Copy link
Member

@BrodiePearson's suggestion of using a single viscosity object is a good one. You might maximize performance with tuples rather than arrays (since the length of a tuple is known statically, you can dispatch / unroll loops over tuples).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants