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 terminal velocities to P3 scheme #370

Merged
merged 1 commit into from
Apr 19, 2024
Merged

Add terminal velocities to P3 scheme #370

merged 1 commit into from
Apr 19, 2024

Conversation

trontrytel
Copy link
Member

Continuing from #334

Copy link

codecov bot commented Apr 13, 2024

Codecov Report

Merging #370 (0945be5) into main (01e6c31) will decrease coverage by 0.07%.
Report is 1 commits behind head on main.
The diff coverage is 95.19%.

❗ Current head 0945be5 differs from pull request most recent head ca4357b. Consider uploading reports for the commit ca4357b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #370      +/-   ##
==========================================
- Coverage   94.95%   94.89%   -0.07%     
==========================================
  Files          36       36              
  Lines        1288     1370      +82     
==========================================
+ Hits         1223     1300      +77     
- Misses         65       70       +5     
Components Coverage Δ
src 98.95% <95.19%> (-0.36%) ⬇️
ext 54.40% <ø> (ø)

@trontrytel trontrytel force-pushed the apaj/p3_termVel branch 2 times, most recently from b89e85f to 6004129 Compare April 18, 2024 22:44
@trontrytel trontrytel marked this pull request as ready for review April 18, 2024 22:45
@trontrytel trontrytel self-assigned this Apr 18, 2024
@trontrytel trontrytel added the P3 label Apr 18, 2024
@trontrytel trontrytel linked an issue Apr 18, 2024 that may be closed by this pull request
Copy link
Contributor

@anastasia-popova anastasia-popova 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 to me! There is a small mistake I made in the docs where I cited Chen 2022 but did not mention it but should be a quick fix.

@trontrytel trontrytel enabled auto-merge April 19, 2024 17:49
@trontrytel trontrytel mentioned this pull request Apr 19, 2024
7 tasks
Copy link
Member

@nefrathenrici nefrathenrici left a comment

Choose a reason for hiding this comment

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

Very readable code, thank you!

Cloudy = "0.4"
DataFrames = "1.6"
DocStringExtensions = "0.8, 0.9"
EnsembleKalmanProcesses = "1.1.5"
ForwardDiff = "0.10"
MLJ = "0.20"
QuadGK = "2.9.4"
Copy link
Member

Choose a reason for hiding this comment

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

Does the compat need to be so precise?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not. - Lets see!

Copy link
Member Author

Choose a reason for hiding this comment

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

But in a separate PR


Integrates f(D, c1, c2, c3) dD from x₀ to x_end
"""
function ∫_Γ(x₀::FT, x_end::FT, c1::FT, c2::FT, c3::FT) where {FT}
Copy link
Member

Choose a reason for hiding this comment

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

This is a great function name!

@@ -9,6 +9,7 @@ DocumenterCitations = "daee34ce-89f3-4625-b898-19384cb65244"
Literate = "98b081ad-f1c9-55d3-8b20-4c87d4299306"
OrdinaryDiffEq = "1dea7af3-3e70-54e6-95c3-0bf5283fa5ed"
Plots = "91a5bcdd-55d7-5caf-9e0b-520d859cae80"
QuadGK = "1fd47b50-473d-5c70-9696-f719f8f3bcdc"
Copy link
Member

Choose a reason for hiding this comment

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

This dependency should only be needed in the main Project.toml, unless you use it in the docs directly. The same applies for the test Project.toml

@trontrytel trontrytel disabled auto-merge April 19, 2024 17:55
@trontrytel trontrytel enabled auto-merge April 19, 2024 18:04
@trontrytel trontrytel merged commit e3a1d83 into main Apr 19, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compute the mass and number weighted terminal velocity
3 participants