-
Notifications
You must be signed in to change notification settings - Fork 50
Add the Long 1974 piecewise collision kernel #1785
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
ekdejong
commented
Jan 8, 2026
- Includes additional functionality in pair methods to support comparison and "where"
- Adds a new option for collision kernel dynamics
- Includes a new unit test in collision kernels
|
thank you @ekdejong !!! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1785 +/- ##
==========================================
+ Coverage 86.76% 86.82% +0.06%
==========================================
Files 414 415 +1
Lines 10485 10536 +51
==========================================
+ Hits 9097 9148 +51
Misses 1388 1388 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@slayoo Can you help me understand the test that is currently failing? I made the linter happy, but it seems like black is still acting. Do I just need to run Black locally? |
black is executed automatically via a pre-commit hook after doing (this is mentioned in the README: https://github.com/open-atmos/PySDM/blob/main/README.md?plain=1#L131-L135, but any feedback on how to better explain the workflow warmly welcome!) |
|
Thank you, @slayoo ! It's been some time since I contributed, clearly...
And speaking of which, thank you for bringing to my attention that the README has some valuable information on contributing! My bad for overlooking this. |
| self.particulator = builder.particulator | ||
| builder.request_attribute("volume") | ||
| builder.request_attribute("radius") | ||
| for key in ( |
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.
we're allocating 8 temporary arrays here, and since some of these are anyhow named just tmp, perhaps we could do fewer allocations, e.g., by noting that v_ratio could be reused instead of copying it to tmp2 (i.e., no need for tmp2).
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 pointing this out! Yes, this attempt to use a shared "pythonic" descriptor rather than separate numba and GPU backends allocates many arrays.
| "tmp", | ||
| "tmp1", | ||
| "tmp2", | ||
| "condition", |
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.
condition would best be of type bool rather than float, 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.
Yes, good point!
| def test_long1974_regimes(radius): | ||
| # arrange | ||
| env = Box(dv=None, dt=None) | ||
| builder = Builder(backend=CPU(), n_sd=radius.size, environment=env) |
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.
adding GPU support here seems to require the two new backend methods, 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.
Indeed, I added those now.
|
|
||
| radius_lg = np.max(radius.data) | ||
|
|
||
| # assert |
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'd suggest adding a basic notebook reproducing any of the figures from Long paper, and a smoke-test executing that notebook and making assertions on the result of a simulation featuring the new kernel
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.
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 decided that replicating figures 10-11 and 13-14 is perhaps even more informative, as it includes comparisons with other relevant kernels like Golovin
| @@ -0,0 +1,72 @@ | |||
| """ | |||
| piecewise kernel from | |||
| [Long 1974](https://doi.org/10.1175/1520-0469%281974%29031%3C1040%3ASTTDCE%3E2.0.CO%3B2) | |||
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.
let's specify precisely which eq. from the paper is implemented here (the paper discusses many kernels)
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.
Eq 11, noted
| """ | ||
|
|
||
|
|
||
| class Long1974: |
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.
BTW, in #1714, @yoctoyotta1024 pointed to the Simmel et al 2002 paper with an alternative mathematical formulation of the Long kernel. There is also a link there to Clara's implementation in CLEO. This could be perhaps a reason to name this class in a more mathematical manner, and offer different flags for calculations as in Long 1974 or as in Simmel et al. 2002? Comparing the two would be a great scenario for a unit 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.
I think for the purpose of this PR (which is selfishly done to enable benchmakring against the Long kernel specifically), I will stick with the original formulation and include an example to compare against Golovin kernel, as is done in the Long paper.
|
(let me also clarify that from all the above comments, adding an example notebook reproducing a figure from the paper is by far my key suggestion here, and I'm happy to help with equipping it later with a smoke test executing the notebook code) |
Thanks @slayoo! I've added a notebook to replicate some of the DSDs in the Long paper. I think an appropriate smoke test could be to confirm the size of the mode in the DSD, and/or to confirm a comparison between the mean size predicted using the Long kernel versus Golovin kernel. |
Thank you @ekdejong ! BTW, fun fact (thanks to @emmacware): Golovin's solution was actually given a bit earlier in the same journal by Safranov, which is why it is called Safranov kernel/solution in astrophysical literature: Safranov, V. S. (1962). "A particular solution of the coagulation equation" [in Russian: "Частный случай решения уравнения коагуляции"]. Bull. Acad. Sci. SSSR Geophys. Ser., 147 (1), 64—67. https://mathnet.ru/dan27172 |
