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

Fixes typo in abstract_scalar_biharmonic_diffusivity_closure.jl #2968

Merged
merged 4 commits into from
Mar 23, 2023

Conversation

tomchor
Copy link
Collaborator

@tomchor tomchor commented Mar 12, 2023

No description provided.

…closure.jl

Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
@tomchor
Copy link
Collaborator Author

tomchor commented Mar 12, 2023

@navidcy's comment made me realize that AbstractScalarDiffusivity has a similar emphasis on isotropic:

"""
abstract type AbstractScalarDiffusivity <: AbstractTurbulenceClosure end
Abstract type for closures with *isotropic* diffusivities.
"""
abstract type AbstractScalarDiffusivity{TD, F} <: AbstractTurbulenceClosure{TD} end

And while that is correct, it may be a bit misleading to users since we can use ScalarDiffusiity to set-up anisotropic diffusivities using Tuples. Should we change to wording there too?

@tomchor tomchor requested a review from navidcy March 12, 2023 14:21
@glwagner
Copy link
Member

@navidcy's comment made me realize that AbstractScalarDiffusivity has a similar emphasis on isotropic:

"""
abstract type AbstractScalarDiffusivity <: AbstractTurbulenceClosure end
Abstract type for closures with *isotropic* diffusivities.
"""
abstract type AbstractScalarDiffusivity{TD, F} <: AbstractTurbulenceClosure{TD} end

And while that is correct, it may be a bit misleading to users since we can use ScalarDiffusiity to set-up anisotropic diffusivities using Tuples. Should we change to wording there too?

Hm no I don't think it's correct. AbstractScalarDiffusivity is for representing turbulence closures that have a scalar diffusivity, not isotropic diffusion (vertical or horizontal formulations are anisotropic).

…closure.jl

Co-authored-by: Gregory L. Wagner <wagner.greg@gmail.com>
@tomchor
Copy link
Collaborator Author

tomchor commented Mar 22, 2023

@navidcy's comment made me realize that AbstractScalarDiffusivity has a similar emphasis on isotropic:

"""
abstract type AbstractScalarDiffusivity <: AbstractTurbulenceClosure end
Abstract type for closures with *isotropic* diffusivities.
"""
abstract type AbstractScalarDiffusivity{TD, F} <: AbstractTurbulenceClosure{TD} end

And while that is correct, it may be a bit misleading to users since we can use ScalarDiffusiity to set-up anisotropic diffusivities using Tuples. Should we change to wording there too?

Hm no I don't think it's correct. AbstractScalarDiffusivity is for representing turbulence closures that have a scalar diffusivity, not isotropic diffusion (vertical or horizontal formulations are anisotropic).

I changed the text according to your comment. But just to note, I said that is technically correct because the naming given there uses the work isotropic when the ν doesn't change in the directions of the formulation:

"""
struct ThreeDimensionalFormulation end
Specifies a three-dimensionally-isotropic `ScalarDiffusivity`.
"""
struct ThreeDimensionalFormulation <: AbstractDiffusivityFormulation end
"""
struct HorizontalFormulation end
Specifies a horizontally-isotropic, `VectorInvariant`, `ScalarDiffusivity`.
"""
struct HorizontalFormulation <: AbstractDiffusivityFormulation end

Which, again, I think it's a bit misleading, but afaik it's technically correct.

@glwagner
Copy link
Member

glwagner commented Mar 22, 2023

It's just that "horizontally isotropic" is not the same thing as "isotropic". What is misleading?

@tomchor
Copy link
Collaborator Author

tomchor commented Mar 22, 2023

It's just that "horizontally isotropic" is not the same thing as "isotropic". What is misleading?

sorry, nvmd, I'm explaning myself poorly. Nothing is misleading anymore since I removed the word "isotropic" in the last commit. We're good to merge whenever tests pass.

@tomchor tomchor merged commit 5e9fbe2 into main Mar 23, 2023
@navidcy navidcy deleted the tc/biharmonic-diff-typo branch March 23, 2023 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants