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

support for AeroMode instantiation with sampled mode type (incl. fixes in spec file logic) + sampled and mono mode types coverage in unit tests (in test_aero_[mode,state,dist].py) #357

Merged
merged 18 commits into from
May 22, 2024

Conversation

slayoo
Copy link
Member

@slayoo slayoo commented Apr 23, 2024

No description provided.

@slayoo slayoo changed the title first try at instantiating an AeroMode with sampled mode type covering AeroMode instantiation with sampled mode type in unit tests Apr 23, 2024
@slayoo slayoo linked an issue Apr 23, 2024 that may be closed by this pull request
@slayoo slayoo changed the title covering AeroMode instantiation with sampled mode type in unit tests covering AeroMode instantiation with sampled mode type in unit tests (+ more examples of AeroMode vs. AeroDist handling) May 7, 2024
@slayoo slayoo force-pushed the aeromode_sampled branch from bdb0270 to 327ecd4 Compare May 17, 2024 08:54
@slayoo slayoo changed the title covering AeroMode instantiation with sampled mode type in unit tests (+ more examples of AeroMode vs. AeroDist handling) covering AeroMode instantiation with sampled mode type in unit tests (both in test_aero_mode and test_aero_dist) May 17, 2024
@slayoo
Copy link
Member Author

slayoo commented May 17, 2024

@jcurtis2, the issue with the new test_sampled test was that

         "size_dist": [
                        {"num_conc": num_concs},
                        {"diam": [1, 2, 3, 4]},
                    ],

and

         "size_dist": [
                        {"diam": [1, 2, 3, 4]},
                        {"num_conc": num_concs},
                    ],

are not the same, because we retrieve the single-element dictionaries based on their placement in the size_dist value (list), and not based on the key. Now, there are checks in place informing user if the order is wrong. I've also added a check depicting that analogous summation of num_conc works if the mode is part of a dist.

Shall we merge?

@slayoo slayoo requested a review from jcurtis2 May 17, 2024 09:23
jcurtis2
jcurtis2 previously approved these changes May 17, 2024
Copy link
Member

@jcurtis2 jcurtis2 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@jcurtis2
Copy link
Member

@jcurtis2, the issue with the new test_sampled test was that

         "size_dist": [
                        {"num_conc": num_concs},
                        {"diam": [1, 2, 3, 4]},
                    ],

and

         "size_dist": [
                        {"diam": [1, 2, 3, 4]},
                        {"num_conc": num_concs},
                    ],

are not the same, because we retrieve the single-element dictionaries based on their placement in the size_dist value (list), and not based on the key. Now, there are checks in place informing user if the order is wrong. I've also added a check depicting that analogous summation of num_conc works if the mode is part of a dist.

Shall we merge?

Just want to note that the order here is different than the PartMC input file order:

!! The resulting size distribution is taken to be piecewise
!! constant in log-diameter coordinates.
!!
!! Example: a size distribution could be:
!! <pre>
!! diam 1e-7 1e-6 1e-5  # bin edge diameters (m)
!! num_conc 1e9 1e8     # bin number concentrations (m^{-3})
!! </pre>
!! This distribution has 1e9 particles per cubic meter with
!! diameters between 0.1 micron and 1 micron, and 1e8 particles
!! per cubic meter with diameters between 1 micron and 10 micron.

But if we have things to check order in PyPartMC, this should be fine.

@slayoo
Copy link
Member Author

slayoo commented May 17, 2024

hmm... let's then first try adding a test that would check if the diams are correctly being stored...

@jcurtis2
Copy link
Member

jcurtis2 commented May 17, 2024

Yeah, at least debugging with some print statements in the PartMC code (inserted in spec_file_read_size_dist https://github.com/compdyn/partmc/blob/e9451e51d845542bb4f586d2d194bf41e7f19af0/src/aero_mode.F90#L795-L875), looks like the diameter bins are incorrect now instead of the number concentrations. Both data arrays in this function are the same.

diameters 100.00000000000000 200.00000000000000 300.00000000000000
num conc 100.00000000000000 200.00000000000000 300.00000000000000

@jcurtis2 jcurtis2 dismissed their stale review May 17, 2024 18:57

Particle size array not correct. need to fix and improved test to verify bin edges or particles sampled.

@slayoo
Copy link
Member Author

slayoo commented May 17, 2024

Thanks Jeff!

@slayoo
Copy link
Member Author

slayoo commented May 17, 2024

BTW, do we have any mean of testing it from Python at this point? (indirectly by sampling from the dist?)

@jcurtis2
Copy link
Member

jcurtis2 commented May 17, 2024

BTW, do we have any mean of testing it from Python at this point? (indirectly by sampling from the dist?)

Not currently - doesn't appear to be anything PartMC function that will do something with it (not like number concentration where there is a function that just gets the total number concentration of a mode aero_mode_total_num_conc). So we can either sample particles and make sure they are within the allowable bin range or we could write something to expose the sample_radius and sample_num_conc of the aero_mode though like we do for other parts like char_radius.

@jcurtis2 jcurtis2 changed the title covering AeroMode instantiation with sampled mode type in unit tests (both in test_aero_mode and test_aero_dist) covering AeroMode instantiation with sampled mode type in unit tests (both in test_aero_mode and test_aero_state) May 22, 2024
@slayoo slayoo changed the title covering AeroMode instantiation with sampled mode type in unit tests (both in test_aero_mode and test_aero_state) support for AeroMode instantiation with sampled mode type (incl. fixes in spec file logic) + sampled and mono mode types coverage in unit tests (both in test_aero_mode and test_aero_state) May 22, 2024
@slayoo slayoo changed the title support for AeroMode instantiation with sampled mode type (incl. fixes in spec file logic) + sampled and mono mode types coverage in unit tests (both in test_aero_mode and test_aero_state) support for AeroMode instantiation with sampled mode type (incl. fixes in spec file logic) + sampled and mono mode types coverage in unit tests (in test_aero_[mode,state,dist]) May 22, 2024
@slayoo slayoo changed the title support for AeroMode instantiation with sampled mode type (incl. fixes in spec file logic) + sampled and mono mode types coverage in unit tests (in test_aero_[mode,state,dist]) support for AeroMode instantiation with sampled mode type (incl. fixes in spec file logic) + sampled and mono mode types coverage in unit tests (in test_aero_[mode,state,dist].py) May 22, 2024
@slayoo
Copy link
Member Author

slayoo commented May 22, 2024

Thanks @jcurtis2
Looks good to me, please confirm if OK to merge (and release)?

Copy link
Member

@jcurtis2 jcurtis2 left a comment

Choose a reason for hiding this comment

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

Looks all good to me!

@slayoo slayoo added this pull request to the merge queue May 22, 2024
Merged via the queue into open-atmos:main with commit d6dbe31 May 22, 2024
37 checks passed
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 this pull request may close these issues.

AeroMode: cover sampled mode_type in tests
2 participants