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.

Sign up for GitHub

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

Jump to bottom

add Clausius_Clapeyron_relation #112

Merged
merged 1 commit into from
Sep 27, 2022
Merged

add Clausius_Clapeyron_relation #112

merged 1 commit into from
Sep 27, 2022

Conversation

yairchn
Copy link
Member

@yairchn yairchn commented Jun 6, 2022

Add the partial derivative of saturation vapor specific humidity with temperature given by the Clausius Clapeyron relation

tapios
tapios requested changes Jun 6, 2022
View reviewed changes
Copy link
Contributor

@tapios tapios left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please introduce a weighted_latent_heat function and go through the thermodynamics code to use this consistently (we are currently defining the same quantity multiple times). (@charleskawczynski).

Please also add a unit test that numerically computes the derivative of q_vap_sat w.r.t. temperature and compares it with this analytical formulation, to ensure consistency.

src/relations.jl Outdated Show resolved Hide resolved
src/relations.jl Outdated Show resolved Hide resolved
charleskawczynski
charleskawczynski reviewed Jun 6, 2022
View reviewed changes
test/relations.jl Outdated Show resolved Hide resolved
@yairchn yairchn force-pushed the yc/Clausius_Clapeyron_relation branch from 3209135 to 7c4adc5 Compare June 6, 2022 18:23
tapios
tapios reviewed Sep 21, 2022
View reviewed changes
src/relations.jl Outdated Show resolved Hide resolved
tapios
tapios reviewed Sep 21, 2022
View reviewed changes
test/relations.jl Outdated Show resolved Hide resolved
@tapios tapios mentioned this pull request Sep 21, 2022
Open
10 tasks
@charleskawczynski charleskawczynski mentioned this pull request Sep 22, 2022
Closed
7 tasks
@charleskawczynski charleskawczynski requested review from tapios and removed request for charleskawczynski September 26, 2022 21:50
@charleskawczynski
Copy link
Member

@tapios, I think this is ready to go if you approve.

Once this is merged, I'll rebase the other PRs and we should hopefully be able to close out a bunch of the open issues.

@codecov
Copy link

codecov bot commented Sep 26, 2022 •
edited
Loading

Codecov Report

Base: 94.00% // Head: 93.55% // Decreases project coverage by -0.44% ⚠️

Coverage data is based on head (f98bab5) compared to base (20a3abf).
Patch coverage: 77.27% of modified lines in pull request are covered.

❗ Current head f98bab5 differs from pull request most recent head 304e398. Consider uploading reports for the commit 304e398 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
- Coverage   94.00%   93.55%   -0.45%     
==========================================
  Files           8        8              
  Lines        1017     1024       +7     
==========================================
+ Hits          956      958       +2     
- Misses         61       66       +5
Impacted Files Coverage Δ
src/relations.jl 96.27% <77.27%> (-0.75%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

tapios
tapios approved these changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@tapios tapios left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

src/relations.jl Outdated Show resolved Hide resolved
src/relations.jl Outdated Show resolved Hide resolved
test/relations.jl Outdated Show resolved Hide resolved
tapios
tapios approved these changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@tapios tapios left a comment

Choose a reason for hiding this comment

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

Looking good. Thank you, @charleskawczynski!

@charleskawczynski
add Clausius_Clapeyron_relation
304e398 
Update src/relations.jl

Co-authored-by: Tapio Schneider <tapio@caltech.edu>

Fix doc strings

liquid_frac -> lambda for consistency

Update ∂q_vap_sat_∂T

Add weighted_latent_heat, simplify ∂q_vap_sat_∂T

Use weighted_latent_heat in ∂e_int_∂T

Use ForwardDiff for test

Update src/relations.jl

Co-authored-by: Tapio Schneider <tapio@caltech.edu>

Inline ice_frac

Use relative tolerance for ∂q_vap_sat_∂T

Fix ∂q_vap_sat_∂T ForwardDiff test

Add docs

Add ForwardDiff to docs env

Fix docs
@charleskawczynski
Copy link
Member

bors r+

@charleskawczynski charleskawczynski mentioned this pull request Sep 27, 2022
Open
@bors
Copy link
Contributor

bors bot commented Sep 27, 2022

Build succeeded:

@bors bors bot merged commit 5ccda2b into main Sep 27, 2022
@bors bors bot deleted the yc/Clausius_Clapeyron_relation branch September 27, 2022 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charleskawczynski charleskawczynski charleskawczynski left review comments

@tapios tapios tapios approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yairchn @charleskawczynski @tapios