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

SurfaceFluxes.jl plugin #24

Merged
merged 16 commits into from
Oct 4, 2021
Merged

SurfaceFluxes.jl plugin #24

merged 16 commits into from
Oct 4, 2021

Conversation

LenkaNovak
Copy link
Collaborator

@LenkaNovak LenkaNovak commented Sep 28, 2021

Provides a coupled test case that uses the dry Nishizawa & Kitamura (2018) formulation using SurfaceFluxes.jl.

TODO:

  • integrate to run with ClimaCore/ ClimaAtmos interface, ensuring type compatibility
  • note changes in SF and NS + revamp the SurfaceFluxes.jl repo with original developers
  • test for longer simtime and compare with the bulk formula test + conservation (conservation achieved)
  • note if still conservation problems due to divergence operator - resolved (needed to apply boundary conditions only to diffusive fluxes, not advective fluxes as was the default)
  • clean up @shows + document the SF functions (+ make README consistent)
  • hook up to CI

Rebased from this PR.

Project.toml Outdated Show resolved Hide resolved
@jb-mackay
Copy link
Contributor

@charleskawczynski can I get your insight on why the Julia formatter is struggling here?

@charleskawczynski
Copy link
Member

Looking...

@charleskawczynski
Copy link
Member

These struct definitions are incomplete:

struct Gravity{𝒯} <: AbstractGravity{𝒯}
struct Buoyancy{𝒯} <: AbstractGravity{𝒯}

@charleskawczynski
Copy link
Member

One nice trick with debugging formatter problems is opening the Julia REPL and trying to load the offending file. This often gives more informative error messages (since the formatter doesn't try to reason about the code in the same way that happens during code loading).

@jb-mackay
Copy link
Contributor

@charleskawczynski does the JuliaFormatter action fail if the formatter hasn't been applied and then the formatted code committed? Or is this action failing for another reason?

Copy link
Contributor

@jb-mackay jb-mackay left a comment

Choose a reason for hiding this comment

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

Looks good - just need the formatter to be happy :)

@charleskawczynski
Copy link
Member

Looks good - just need the formatter to be happy :)

We're not currently depending on the success of the formatter (here), so we can merge this as is. If you'd like to apply the formatter for this PR, we could do that but, since the PR is already pretty large, the formatting could be applied in a separate PR (and then add the format entry in the bors toml file).

@jb-mackay
Copy link
Contributor

bors r+

@charleskawczynski
Copy link
Member

bors ping

@bors
Copy link
Contributor

bors bot commented Oct 4, 2021

pong

@charleskawczynski
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Oct 4, 2021
24: SurfaceFluxes.jl plugin r=charleskawczynski a=LenkaNovak

Provides a coupled test case that uses the dry Nishizawa & Kitamura (2018) formulation using `SurfaceFluxes.jl`. 

## TODO:
- [x] integrate to run with ClimaCore/ ClimaAtmos interface, ensuring type compatibility
- [x] note changes in SF and NS + revamp the SurfaceFluxes.jl repo with original developers
- [x] test for longer simtime and compare with the bulk formula test + conservation (conservation achieved)
- [x] note if still conservation problems due to divergence operator - resolved (needed to apply boundary conditions only to diffusive fluxes, not advective fluxes as was the default)
- [x] clean up `@shows` + document the SF functions (+ make README consistent)
- [x] hook up to CI 

Rebased from [this](#19) PR.

Co-authored-by: LenkaNovak <lenka@caltech.edu>
Co-authored-by: Ben Mackay <jbmackay@caltech.edu>
@charleskawczynski
Copy link
Member

bors r-

@bors
Copy link
Contributor

bors bot commented Oct 4, 2021

Canceled.

@jb-mackay
Copy link
Contributor

bors ping

@bors
Copy link
Contributor

bors bot commented Oct 4, 2021

pong

@jb-mackay
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 4, 2021

@bors bors bot merged commit b667586 into main Oct 4, 2021
@bors bors bot deleted the ln/surface-fluxes-plugin2 branch October 4, 2021 23:22
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.

3 participants