FourierFlows / FourierFlows.jl

Tools for building fast, hackable, pseudospectral partial differential equation solvers on periodic domains
https://bit.ly/FourierFlows
MIT License
206 stars 29 forks source link

Add clarification in Docs #226

Closed navidcy closed 3 years ago

navidcy commented 3 years ago

[moved here from https://github.com/FourierFlows/GeophysicalFlows.jl/issues/132; thanks @francispoulin]

Some thoughts on the docs. Please ignore whatever you like, since you are the experts of ourse.

navidcy commented 3 years ago

hey @francispoulin, sorry for slacking on this.

Regarding,

In the range of k maybe put a cap fo n_x/2 -1 or something? Since it's necessarily finite.

I disagree that k is finite at that point. If x ∈ [0, L] then the allowed wavenumber are k = (2π/L){0, ±1, ±2, ...}. It's only when x becomes discrete, i.e., x = (j-1)2π/L, j=1,...,n that only n of the above wavenumber are independent and thus the sum should be capped.

navidcy commented 3 years ago

PR #221 dealt with some of the issues regarding Grids section

navidcy commented 3 years ago

Maybe mention that in most systems L is a 1D array, so a vector?

L has the same dimensions as prob.sol it's not really 1D most of the times.

navidcy commented 3 years ago

It's great that Julia gives you the output after grid = OneDGrid(nx, Lx). What is this called?

Actually this is custom made way to show the Grid struct. See

https://github.com/FourierFlows/FourierFlows.jl/blob/d664b6a05dbcaf89a1dac3cf9e5c63bc8657dcda/src/domains.jl#L398-L426

navidcy commented 3 years ago

You direct the reader to Diffusion in src but I don't see it there. Should it be?

It's there: https://github.com/FourierFlows/FourierFlows.jl/blob/d664b6a05dbcaf89a1dac3cf9e5c63bc8657dcda/src/diffusion.jl#L1

navidcy commented 3 years ago

I presume three is a way to list the variables in prob.vars. Can you state that? This case there is only u and uh, but in SW there would be several.

I don't understand exactly what you mean. Even in this example there are already 2 variables in vars: u and uh. Do you think a clarification is needed?

navidcy commented 3 years ago

Are Aphys and Atrans place holders or do they mean something in particular?

This are parametric types so that, e.g., u can be of type Array{Float64, 2} or Array{Float32, 2} or even CuArray{Float32, 2}... Some explanation is needed. But perhaps it'll come a bit too much at this stage? @glwagner what do you think?

francispoulin commented 3 years ago

hey @francispoulin, sorry for slacking on this.

Regarding,

In the range of k maybe put a cap fo n_x/2 -1 or something? Since it's necessarily finite.

I disagree that k is finite at that point. If x ∈ [0, L] then the allowed wavenumber are k = (2π/L){0, ±1, ±2, ...}. It's only when x becomes discrete, i.e., x = (j-1)2π/L, j=1,...,n that only n of the above wavenumber are independent and thus the sum should be capped.

True for this early stage. Maybe you could add a sentence pointing that out? I think it could help the reader when moving from the continuous to the discrete versions of the equations, which you guide them through in the docs.

francispoulin commented 3 years ago

I presume three is a way to list the variables in prob.vars. Can you state that? This case there is only u and uh, but in SW there would be several.

I don't understand exactly what you mean. Even in this example there are already 2 variables in vars: u and uh. Do you think a clarification is needed?

Sorry I was unclear. I guess I was thinking far into the future, maybe too far, and thought given a model the reader might want to know what fields are contained in the models. Yesterday, I learend that you can do something like keys(fields(model)) and that gives you a list of all the fields in model. This is something that I wanted to know back whwen I read it first, but I am not convinced this is a general concern.

francispoulin commented 3 years ago

Are Aphys and Atrans place holders or do they mean something in particular?

This are parametric types so that, e.g., u can be of type Array{Float64, 2} or Array{Float32, 2} or even CuArray{Float32, 2}... Some explanation is needed. But perhaps it'll come a bit too much at this stage? @glwagner what do you think?

I agree that you don't want to burdern the reader with all of the information. I don't think you need to change anything here.

francispoulin commented 3 years ago

Thanks @navid for the thoughtful reply, it is appreciated. I do look forward to getting back to FourierFlows after I have some shallow water stuff running on Oceananigans. I'd like to get a Shallow Water version in both, but we will see how that goes.

I am happy for you to close this issue whenever you like. It has been very helpful for me and do appreciate it.

navidcy commented 3 years ago

hey @francispoulin, sorry for slacking on this. Regarding,

In the range of k maybe put a cap fo n_x/2 -1 or something? Since it's necessarily finite.

I disagree that k is finite at that point. If x ∈ [0, L] then the allowed wavenumber are k = (2π/L){0, ±1, ±2, ...}. It's only when x becomes discrete, i.e., x = (j-1)2π/L, j=1,...,n that only n of the above wavenumber are independent and thus the sum should be capped.

True for this early stage. Maybe you could add a sentence pointing that out? I think it could help the reader when moving from the continuous to the discrete versions of the equations, which you guide them through in the docs.

Done.