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

Add default Ω_Earth and R_Earth for FPlane and BetaPlane #550

Merged
merged 6 commits into from
Dec 14, 2019

Conversation

ali-ramadhan
Copy link
Member

Resolves #510

@ali-ramadhan ali-ramadhan added the feature 🌟 Something new and shiny label Dec 4, 2019
@glwagner
Copy link
Member

glwagner commented Dec 9, 2019

How many significant digits would we like to use? The number of digits in each constant is not consistent.

@ali-ramadhan
Copy link
Member Author

How many significant digits would we like to use? The number of digits in each constant is not consistent.

I think we should use as many significant digits as possible up to experimental uncertainty. Why should different constants have the same number of significant digits, especially if they're not always combined?

For Ω_Earth we have 7.2921150 ± 0.0000001 × 10^−5 radians per SI second so I updated this PR to use 7.292115e-5.

For R_Earth the Handbook of Chemistry and Physics quotes 6371.0 km for the mean radius of the Earth so I updated the PR to use 6371.0e3.

@glwagner
Copy link
Member

glwagner commented Dec 12, 2019

I guess that's a fair point. I don't think we use all the significant digits available from experiments for g_Earth. Just thought it could be nice to make a decision so we don't end up making arbitrary truncations in cases where constants are known to 42 digits, but we decide not to copy 42 digits into the source code.

Right --- R_Earth is a bit ambiguous.

But if the values of such constants are super important to users simulations, its probably best to encourage them to set them explicitly in scripts.

But, hardly matters at this point since all we do are rectangles...

@ali-ramadhan
Copy link
Member Author

Just thought it could be nice to make a decision so we don't end up making arbitrary truncations in cases where constants are known to 42 digits, but we decide not to copy 42 digits into the source code. Kind of happened with the gas constant: CliMA/ClimateMachine.jl#496

I think in these cases we should copy over all 42 digits.

But if the values of such constants are super important to users simulations, its probably best to encourage them to set them explicitly in scripts.

But, hardly matters at this point since all we do are rectangles...

True.

If it turns out to be a bad idea to assume defaults then we can take them away.

@codecov
Copy link

codecov bot commented Dec 14, 2019

Codecov Report

Merging #550 into master will increase coverage by 3.86%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #550      +/-   ##
==========================================
+ Coverage   67.72%   71.59%   +3.86%     
==========================================
  Files          69       71       +2     
  Lines        1952     2341     +389     
==========================================
+ Hits         1322     1676     +354     
- Misses        630      665      +35
Impacted Files Coverage Δ
src/coriolis.jl 60.86% <100%> (ø) ⬆️
src/boundary_conditions.jl 69.06% <0%> (-4.97%) ⬇️
src/halo_regions.jl 83.05% <0%> (-3.07%) ⬇️
src/TimeSteppers/kernels.jl 57.8% <0%> (-2.6%) ⬇️
src/AbstractOperations/computations.jl 75.75% <0%> (-0.25%) ⬇️
src/Solvers/batched_tridiagonal_solver.jl 100% <0%> (ø)
src/logger.jl 0% <0%> (ø)
src/Oceananigans.jl 76.92% <0%> (+1.92%) ⬆️
src/OutputWriters/netcdf_output_writer.jl 89.23% <0%> (+2.69%) ⬆️
src/utils.jl 74.35% <0%> (+2.79%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a511fdd...938eb44. Read the comment docs.

@ali-ramadhan ali-ramadhan merged commit e15ef87 into master Dec 14, 2019
@ali-ramadhan ali-ramadhan deleted the ar/default-f-plane branch December 14, 2019 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🌟 Something new and shiny
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ωᵉᵃʳᵗʰ default for Coriolis constructors?
2 participants