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

AeroDist constructor ignores all but last mode of the distribution (including in the README example) #393

Open
Griger5 opened this issue Jan 10, 2025 · 1 comment · May be fixed by #394

Comments

@Griger5
Copy link
Collaborator

Griger5 commented Jan 10, 2025

Code example:

import numpy as np
import PyPartMC as ppmc
from PyPartMC import si
import math

aero_data = ppmc.AeroData((
    {"OC": [1000 *si.kg/si.m**3, 0, 1e-3 *si.kg/si.mol, 0.001]},
))

common = {
    "mass_frac": [{"OC": [1]}],
    "diam_type": "geometric",
    "mode_type": "log_normal",
    "geom_mean_diam": 8.64 * si.nm,
    "log10_geom_std_dev": 0.28,
}

M1 = {
    "num_conc": 1 / si.cm**3,
    **common
}

M2 = {
    "num_conc": 1e10 / si.cm**3,
    **common
}

D1 = ppmc.AeroDist(aero_data, [{"mode1":M1}])
D2 = ppmc.AeroDist(aero_data, [{"mode2":M2}, {"mode1":M1}])

n_part = 100
masses = []

for d in (D1, D2):
    aero_state = ppmc.AeroState(aero_data, n_part, "nummass_source")
    aero_state.dist_sample(d)

    masses.append(np.dot(aero_state.masses(), aero_state.num_concs))

assert not math.isclose(masses[0], masses[1], rel_tol=0.9)

D1 and D2 should produce different results after using dist_sample. The problem is caused by both AeroDists being initialized with only the M1 mode. The assertion in code above should pass when the bug is fixed.

NOTE: There's no issue if you initialize D2 with [{"mode2":M2, "mode1":M1}].

@slayoo
Copy link
Member

slayoo commented Jan 11, 2025

Thank you @Griger5!

So, IIUC, we have a bug in the README code here:

PyPartMC/README.md

Lines 112 to 115 in 88bc802

}
},
{
"diesel": {

(should be just ",").

AeroDist is used in the correct way in multiple examples notebooks (no of the notebooks seem to have this bug), e.g.:

" },\n",
" \"init_large\": {\n",

Similarly, the correct usage is featured in unit tests, e.g.:

},
"test_mode_B": {

The bug causes the README code to ignore the first mode.
Here's a change that should fix the README bug: #394

The concept of checking input spec-file data for being used entirely by PyPartMC, proposed in #54, will make such bug turn into a Python error with an understandable message to the user. As you've discovered the bug working on #54, it shows how important is to have this feature.

The test case you provided is great, and will be a good test to include in a PR for #54 to clearly depict why we need it.

@slayoo slayoo linked a pull request Jan 20, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants