Seeding dynamic and associated initialisation logic (super-droplet injection during simulation)

[slayoo]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.61%. Comparing base (723e62c) to head (794243c).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1367      +/-   ##
==========================================
+ Coverage   84.30%   84.61%   +0.30%     
==========================================
  Files         373      375       +2     
  Lines        9160     9247      +87     
==========================================
+ Hits         7722     7824     +102     
+ Misses       1438     1423      -15     

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

[claresinger]

Some sanity check tests to add in might be 1) checking that turning on Seeding dynamic but having time_window=0 yields the same results as no seeding, 2) checking that all particles have multiplicity > 0 by end of seeding window, and then a more physical check would be to check that the droplet number concentrations are larger and the droplet sizes are smaller when you have seeding.

Hopefully these will address the warnings from Codecov :)

[slayoo]

In 292d4fb, there is a seeding/no-seeding plot layout depicting what happens also without seeding

[claresinger]

@slayoo I tried to write a unit test for the Seeding dynamic. It's here. https://github.com/claresinger/PySDM/blob/sd_injection/tests/unit_tests/dynamics/seeding/test_parcel_seeding_sanity_checks.py
I'm not sure I have permissions to commit changes to this PR... or at least I didn't figure out how to do that, sorry!

[slayoo]

@claresinger, thanks!
I'd vote for placing this type of test into smoke_tests rather than unit_tests - it checks much more than just the seeding dynamic "unit" (and it takes much more time than a unit test). Also, we could avoid copy-pasting code and run the hello_world notebook via the notebook_vars fixture and assert on its variables (like we did here: https://github.com/open-atmos/PySDM/blob/main/tests/smoke_tests/parcel_a/lowe_et_al_2019/test_fig_s2.py)?

To push to this PR's branch, you should be able to do:

git remote add slayoo git@github.com:slayoo/PySDM.git
git push slayoo sd_injection:sd_injection 

[slayoo]

great that the push worked, thanks!
I'll try to reduce code duplication in the smoke test now

[slayoo]

BTW, adding a new folder in smoke tests requires also adding an entry here:

PySDM/.github/workflows/tests+artifacts+pypi.yml

Line 128 in 55e0cce

test-suite: ["unit_tests/!(dynamics)", "unit_tests/dynamics/!(condensation)", "unit_tests/dynamics/condensation", "smoke_tests/no_env", "smoke_tests/box", "smoke_tests/parcel_a", "smoke_tests/parcel_b", "smoke_tests/parcel_c", "smoke_tests/kinematic_1d", "smoke_tests/kinematic_2d", "tutorials_tests"]

[slayoo]

Also, we could avoid copy-pasting code and run the hello_world notebook via the notebook_vars fixture and assert on its variables...

Looking close, I'd say we can better simplify the test to just use a Box env and a few timesteps, without invoking all the notebook machinery. But let me look at it tomorrow

[slayoo]

Also, we could avoid copy-pasting code and run the hello_world notebook via the notebook_vars fixture and assert on its variables...

Looking close, I'd say we can better simplify the test to just use a Box env and a few timesteps, without invoking all the notebook machinery. But let me look at it tomorrow

here it is: d3318e6

[claresinger]

Remaining items, to be handled in another PR, are documented in issue #1387

[claresinger]

Pylint is passing. ✅
But the tests that are passing locally are failing from a RuntimeWarning. ❌

[slayoo]

Pylint is passing. ✅ But the tests that are passing locally are failing from a RuntimeWarning. ❌

Thank you, @claresinger !
In the workflow, we invoke pytest with the -We option to treat warnings as errors

[claresinger]

I think the problem is that discretise_multiplicities does not expect all values to be NaN, only some. But that excludes the case where there are no initial particles, only seed particles, like we are testing.

I think this is a valid case, so we should update the function and add a unit test for this case.

[slayoo]

I think the problem is that discretise_multiplicities does not expect all values to be NaN, only some. But that excludes the case where there are no initial particles, only seed particles, like we are testing.

I think this is a valid case, so we should update the function and add a unit test for this case.

Good point! Thank you!

[slayoo]

codecov reports that the only single line missing test coverage is now (particulator.py):

+         if n_null == 0:
+             raise ValueError(
+                 "No available seeds to inject. Please provide particles with nan filled attributes."
+             )

[slayoo]

And here's a pattern we could use to actually check what type of exception is thrown in case of errors and if the exception message matches some expected string: https://stackoverflow.com/a/77005620/1487910

[jtbuch]

codecov reports that the only single line missing test coverage is now (particulator.py):

+         if n_null == 0:
+             raise ValueError(
+                 "No available seeds to inject. Please provide particles with nan filled attributes."
+             )

Does it make sense to include a case with both background and seed SDs in the backend test to ensure this coverage?

[slayoo]

working on the tests now...

[claresinger]

Pending pylint fixes and passing the new tests, I think this looks great!

[slayoo]

@claresinger, @jtbuch, we've made it!

[jtbuch]

Just ran the code and everything looks good to me!