-
Notifications
You must be signed in to change notification settings - Fork 47
Added example based on Loftus and Wordsworth 2021 figure 2 #1648
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
base: main
Are you sure you want to change the base?
Conversation
@kaitlyn-loftus, FYI - new example being developed for PySDM - kudos @lursz, @emmacware, @piotrkubala, @Hevagog & @mzsuetam! |
great news! happy to share any code or try to help out!! there are also a bunch of functions for properties of a CO2 atmosphere here: https://github.com/kaitlyn-loftus/serious-cirrus/blob/main/src/%CE%9C%CE%A6.jl |
class Earth(Planet): | ||
g_std: float = 9.82 * si.metre / si.second**2 | ||
T_STP: float = 290 * si.kelvin | ||
p_STP: float = 1.01325 * 1e6 * si.pascal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to hardcode all constants? cannot we used one set in library with better approximation f.e. scipy.constants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scipy
doesn't have mars, saturn, etc. constants, so for single convention and for readability sake I'd argue it is not needed. I see no value added when we have to specifically import scipy
only in the Earth planet and not for others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi all, guess I am subscribed to email notifications on this thread.
I think p_STP should be
p_STP: float = 1.01325 * 1e5 * si.pascal
not
p_STP: float = 1.01325 * 1e6 * si.pascal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi all, guess I am subscribed to email notifications on this thread.
Github notifies anyone who has been @-mentioned in a PR
I think p_STP should be
p_STP: float = 1.01325 * 1e5 * si.pascal
not
p_STP: float = 1.01325 * 1e6 * si.pascal
thanks!
we can also use si.hPa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, we'll fix it right away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T_STP: float = 300 * si.kelvin | ||
p_STP: float = 1.01325 * 1e6 * si.pascal | ||
RH_zref: float = 0.75 | ||
dry_molar_conc_H2: float = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add some comments to clarify what are those variables and why they have such a values (they are just constants or you toke them from another paper)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the proposition, fixed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current tests cover only valid input scenarios. To improve robustness, consider adding edge case tests, such as:
- r_wet < 0
- mass_of_dry_air < 0
- Extremely low/high values for Tcloud or pcloud
with pytest.raises(ValueError):
Settings(
...
)
def advance_parcel_vars(self): | ||
""" | ||
Compute new values of displacement, dry-air density and volume, | ||
and write them to self._tmp and self.mesh.dv | ||
""" | ||
dt = self.particulator.dt | ||
formulae = self.particulator.formulae | ||
T = self["T"][0] | ||
p = self["p"][0] | ||
|
||
dz_dt = -self.particulator.attributes["terminal velocity"].to_ndarray()[0] | ||
water_vapour_mixing_ratio = ( | ||
self["water_vapour_mixing_ratio"][0] | ||
- self.delta_liquid_water_mixing_ratio / 2 | ||
) | ||
|
||
drho_dz = formulae.hydrostatics.drho_dz( | ||
p=p, | ||
T=T, | ||
water_vapour_mixing_ratio=water_vapour_mixing_ratio, | ||
lv=formulae.latent_heat_vapourisation.lv(T), | ||
d_liquid_water_mixing_ratio__dz=( | ||
self.delta_liquid_water_mixing_ratio / dz_dt / dt | ||
), | ||
) | ||
drhod_dz = drho_dz | ||
|
||
self.particulator.backend.explicit_euler(self._tmp["z"], dt, dz_dt) | ||
self.particulator.backend.explicit_euler( | ||
self._tmp["rhod"], dt, dz_dt * drhod_dz | ||
) | ||
|
||
self.mesh.dv = formulae.trivia.volume_of_density_mass( | ||
(self._tmp["rhod"][0] + self["rhod"][0]) / 2, self.mass_of_dry_air | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is nearly identical to Parcel.advance_parcel_vars(), differing only in the computation of dz_dt. Consider extracting dz_dt logic into a protected method like _compute_dz_dt() in the base class and overriding it here. This would eliminate duplication and improve maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea 👍.
Fixed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parametrize the test
Firstly, in my opinion, the test is thorough and checks the critical part of your model. Great work on that! 👍🏼
With that being said, I've noticed that to achieve a better readability, you could make use of @pytest.mark.parametrize
decorator and parametrize the test_figure_2_replication_accuracy
function.
@pytest.mark.parametrize(...)
def test_figure_2_replication_accuracy(self):
## code
To make it work, I believe that you should also somehow extract the sampling logic (that happens directly in the test) to the @pytest.fixture
:
@pytest.fixture
def sample_groundtruth_cases():
## code
You can read some more on the use of fixtures here: pytest-fixtures
From my point of view, these changes will result in:
- Cleaner code (makes the code more 'pythonic')
- Separation of concerns (decoupling sampling logic from the assertion logic)
- Easier test readability and debugging (each parametrized case will show as a separate test)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in this commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code documentation
As the notebook is nicely constructed and easy to read, I think that some additional context as to what this example represents would be much appreciated 🙏🏼
As an example, you could perhaps provide some information about the data being used here:
radius_array = np.logspace(-4.5, -2.5, 50) * si.m
RH_array = np.linspace(0.25, 0.99, 50)
output_matrix = np.full((len(RH_array), len(radius_array)), np.nan)
I think that some short, markdown instructions/explanations would be more than enough for potential users, to dive into the code more quickly.
Here is an example of a notebook from PySDM about Kinetic Limitations in which the comments explain the use-case.
} | ||
|
||
self.save(output) | ||
while self.particulator.environment["z"][0] > 0 and output["r"][-1] > 1e-6: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be helpful to replace the magic number 1e-6 in stopping condition with a named constant so that its meaning is clear and it can be easily adjusted if needed. Also you can add comments explaining why this threshold was chosen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. Now the name
MIN_DROPLET_RADIUS
is used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall very good job guys
p = self["p"][0] | ||
|
||
dz_dt = -self.particulator.attributes["terminal velocity"].to_ndarray()[0] | ||
water_vapour_mixing_ratio = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using hardcoded index [0] suggests that the parcel assumes a single particle or layer and can behave incorrectly if any changes occur. Could the length of arrays be asserted here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually the code we copied from PySDM's parcel.py
file.
As I am implementing the solution proposed by @makowskitomasz (using _compute_dz_dt
function; this commit), this PR will not be a good place for fixing that potential issue.
Zcloud, | ||
pcloud, | ||
) | ||
except Exception as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception logs a warning but does not assert fail, should it be handled with pytest.fail() instead/in addition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're absolutely right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in this commit
CI fails de to pre-commit being neither installed or run:
For help, see: https://github.com/open-atmos/python-dev-hints/wiki/Pre-commit-FAQ |
* simulation setup * uv lock * introduced all equations * oblate spheroid impl * AlienParcel class extending Parcel * PySDM package internal imports fix * Formulae bug fixes * Planet is now dataclass instead of dict * drop falling without pancake shape * cloudbase fix * figure settings * multicore support for simulation (#2) * parallel v1 * thread count fix * parallel * small parallel fixes * working multithread solution * Multi-thread + vector plots * black, black used everywhere --------- Co-authored-by: Piotr Kubala <pkubala@student.agh.edu.pl> * pip install joblib * Tests (#1) * tests I guess * uv fix * planet impl * ground truth * gen plot function * unit tests, monoticity tests, sanity tests * tests are now in tests * added ground truth files * removed leftovers * npy files * tests tidyup * tests adapted for current structure but still useless * renamed test to comply with pytest convention * fix ground_truth generation script * refactor unittests * added accuracy tests * Added saturation test for cloud base in unit tests * Refactored imports and clean up unused code in simulation and parcel modules * Enhanced accuracy tests by adding assertions for output completeness and consistency in unit tests * Refactored calculation of simulated mass fraction evaporation point to initialize variable and improved error handling --------- Co-authored-by: Hevagog <qkawiak@gmail.com> Co-authored-by: Mateusz Mazur <mazurm@student.agh.edu.pl> * Notebook not running fix - added back necessary imports m8 u removed too much * black formatter * black for ipynb * Update PySDM/physics/particle_shape_and_density/oblate_spheroid.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Updated tests/examples_tests/Loftus_and_Wordsworth_2021/ground_truth/gen_figure.py Using dir shadowed the built-in dir() function. Renamed this variable (e.g., root_dir) to avoid confusion. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fixed pressure values in Planet subclasses to use correct scientific notation * Updated method signature for proper binding. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Refactored Settings class to remove unused 'coord' parameter and updated diffusion_coordinate assignment * Removed unused Lofus et al. 2021 implementations from particle shape and density, terminal velocity, and ventilation modules * plots warnings fixes * removed files rogers and yau * notebook rerun * notebook fix * minor linter fixes --------- Co-authored-by: emmacware <ecware@ucdavis.edu> Co-authored-by: Piotr Kubala <pkubala@student.agh.edu.pl> Co-authored-by: Hevagog <qkawiak@gmail.com> Co-authored-by: Mateusz Mazur <mazurm@student.agh.edu.pl>
…lanetary pressure units
Introduced const for MIN DROPLET RADIUS
Fixed. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1648 +/- ##
=======================================
Coverage 85.66% 85.66%
=======================================
Files 403 403
Lines 9771 9773 +2
=======================================
+ Hits 8370 8372 +2
Misses 1401 1401 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Added the notebook and utils for recreating figure 2 from Loftus and Wordsworth 2021.
The changes: