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

SingleLayerQG #160

Merged
merged 105 commits into from
Dec 18, 2020
Merged

SingleLayerQG #160

merged 105 commits into from
Dec 18, 2020

Conversation

liasiegelman
Copy link
Collaborator

@liasiegelman liasiegelman commented Dec 4, 2020

This PR renames the BarotropicQG module to SingleLayerQG and adds the ability to include finite Rossby radius of deformation. Also takes the chance to refactor/optimize certain features.

Closes #137.

src/singlelayerqg.jl Outdated Show resolved Hide resolved
@navidcy navidcy requested a review from glwagner December 17, 2020 12:12
@navidcy
Copy link
Member

navidcy commented Dec 17, 2020

@glwagner, me and @liasiegelman have worked on this PR. We feel it's time for somebody else to have a look. What do you think?

@navidcy
Copy link
Member

navidcy commented Dec 17, 2020

@glwagner, there is no preview of docs (something related with Documenter ssh keys and what not). But you can preview the docs locally if you run

julia --project=docs/ -e 'using Pkg; Pkg.instantiate(); Pkg.develop(PackageSpec(path=pwd()))'; julia --project=docs/ docs/make.jl; open docs/build/index.html

from the repo's home directory.

@navidcy
Copy link
Member

navidcy commented Dec 17, 2020

singlelayerqg_barotropic_equivalentbarotropic

@liasiegelman
Copy link
Collaborator Author

@navidcy that's neat. The title for the right panel should probably read \Lap \psi - psi/L_d^2 if it is sol that you are plotting

@navidcy
Copy link
Member

navidcy commented Dec 17, 2020

The animation indeed shows ∇²ψ for both cases.

@liasiegelman
Copy link
Collaborator Author

oki :)

@glwagner
Copy link
Member

I'll take a look!

const BarotropicQGParams = Params{<:AbstractFloat, <:AbstractArray, <:AbstractArray, Nothing}
const EquivalentBarotropicQGParams = Params{<:AbstractFloat, <:AbstractArray, <:AbstractArray, <:AbstractFloat}

get_topographicPV_grid_values(eta::Function, grid::AbstractGrid{T, A}) where {T, A} = A([eta(grid.x[i], grid.y[j]) for i=1:grid.nx, j=1:grid.ny])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe out of scope, but this works for any function of x, y (not just topographic PV), so you could put this in FourierFlows utils.jl or something and instead call it

on_grid(func, grid::TwoDimGrid{T, A}) where {T, A} = ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Let's do it in a separate PR.

eta_grid = typeof(eta) <: AbstractArray ? eta : get_topographicPV_grid_values(eta, grid)
eta_gridh = rfft(eta_grid)
return Params(β, nothing, eta_grid, eta_gridh, μ, ν, nν, calcF)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be

BarotropicQGParams(grid::AbstractGrid{T, A}, β, eta, μ, ν, nν::Int, calcF) where {T, A} =
    EquivalentBarotropicQGParams(grid, β, nothing, eta, μ, ν, nν, calcF)


"""
set_q!(prob, q)
set_q!(sol, vars, params, grid)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it makes sense just to document the user API here via set_q!(prob, q) rather than also including the "lower-level" form.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note there's also a typo in the lower level form, should be set_q!(sol, vars, params, grid, q)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are suggesting that set_q!(sol, vars, params, grid, q) is useless and we just need to have set_q!(prob, q)? I think I agree... I don't remember where set_q!(sol, vars, params, grid, q) was useful for... perhaps legacy now...

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've done a great job with this! Great work all!

@navidcy navidcy changed the title [WIP] SingleLayerQG SingleLayerQG Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤥 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add 1.5-layer QG functionality
3 participants