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

Refactor parcel #331

Merged
merged 1 commit into from
Feb 24, 2024
Merged

Refactor parcel #331

merged 1 commit into from
Feb 24, 2024

Conversation

trontrytel
Copy link
Member

@trontrytel trontrytel commented Feb 21, 2024

Purpose

Refactor the adiabatic parcel code to get rid of convoluted if/else logic that frew on us

Content

The parcel model code is now structured into

  • Parcel.jl - includes all the files. Needs to be included in all examples
  • ParcelCommon.jl - defines some functions that are reused when computing initial conditions, etc.
  • ParcelDistributions.jl - computes the mean radius, area and volume for particles for monodisperse, Gamma and Lambda distributions
  • ParcelModel.jl - defines the parcel problem for ODE solver
  • ParcelParameters.jl - creates structs with parameters that will be used to dispatch over different parameterizations
  • ParcelTendencies.jl - defines different ice nucleation and growth tendencies. Note that for each of them the first argument is a parameter struct that serves as type for dispatching.

To-do

Couple of problems that I noticed while coding

  • I think there is something wrong with the Lognormal distribution. It seems like it is not connected at all with the predicted ql and Nl. Is it meant to be used only for background aerosol?
  • In a couple of places we were using ^3 instead of <r^3>. For example here. This should be fixed now thanks to the ParcelDistributions
  • The Jensen example gives different results than before. I think it is because of the issues with the Lognormal size distribution.

Copy link

codecov bot commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.72%. Comparing base (bdaa5d6) to head (b7d6097).
Report is 1 commits behind head on main.

❗ Current head b7d6097 differs from pull request most recent head 234754b. Consider uploading reports for the commit 234754b to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #331   +/-   ##
=======================================
  Coverage   98.72%   98.72%           
=======================================
  Files          33       33           
  Lines        1101     1101           
=======================================
  Hits         1087     1087           
  Misses         14       14           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@trontrytel trontrytel self-assigned this Feb 23, 2024
@trontrytel trontrytel marked this pull request as ready for review February 23, 2024 00:19
@trontrytel trontrytel changed the title wip on refactoring parcel Refactor parcel Feb 23, 2024
Copy link
Member

@amylu00 amylu00 left a comment

Choose a reason for hiding this comment

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

Thank you so much Anna! It looks beautiful!

  • I think you may have used the deposition water activity based parameterization for Jensen instead of the homogeneous one. Maybe those can be renamed to avoid confusion in the future. This might fix the problem of the plot not looking the same but let me know if otherwise. Jensen also assumes the aerosols are lognormally distributed and not the droplets so that may also be affecting the results.
  • I think lognormal droplets account for q_liq and N_liq through R_mode_liq and σ. I'll see if I can find out how they relate and fix it. For now, none of the files actually use it so it's fine but maybe we should have it throw a warning? I'll open an issue rn

@trontrytel trontrytel requested a review from amylu00 February 23, 2024 23:21
@trontrytel trontrytel force-pushed the aj/parcel_refactor branch 2 times, most recently from 6bbc39e to 234754b Compare February 23, 2024 23:30
Copy link
Member

@amylu00 amylu00 left a comment

Choose a reason for hiding this comment

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

I love it

@trontrytel trontrytel enabled auto-merge February 24, 2024 00:02
@trontrytel trontrytel merged commit 63b4804 into main Feb 24, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants