-
Notifications
You must be signed in to change notification settings - Fork 7
Adds initial external aerosol model option #910
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
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.
Pull request overview
This PR adds initial support for integrating external aerosol models into MICM solvers by introducing a new ExternalModel type that wraps external model objects and captures their state size and naming functions. The implementation removes the phases_ and others_ members from the System class, replacing them with the more flexible external_models_ vector. Additionally, the PR removes the requirement for solvers to have at least one process, enabling solver creation for testing and scenarios where external models provide all state variables.
Changes:
- Introduced
ExternalModelstruct to wrap external models via function pointers - Modified
Phase::UniqueNames()to include phase name prefix and addedSpeciesNames()for backward compatibility - Removed
phases_andothers_members fromSystemclass and related validation logic - Removed requirement for solvers to have processes (deleted
valid_reactions_flag andMissingProcesseserror check)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| include/micm/system/system.hpp | Added ExternalModel struct with function pointers; updated System class to use external_models_ instead of phases_/others_ |
| include/micm/system/phase.hpp | Modified UniqueNames() to include phase prefix; added SpeciesNames() for names without prefix |
| include/micm/solver/solver_builder.hpp | Removed valid_reactions_ flag |
| include/micm/solver/solver_builder.inl | Removed process validation logic and phases_ tolerance handling |
| test/unit/system/test_system.cpp | Updated tests to remove phases_ and others_ usage; removed OthersIsStoredAndAccessible test |
| test/unit/system/test_phase.cpp | Updated test expectations to match new UniqueNames() behavior with phase prefix |
| test/unit/solver/test_state.cpp | Updated tests to remove others_ usage |
| test/unit/solver/test_solver_builder.cpp | Removed ThrowsMissingReactions test and phases_ assertions |
| test/integration/test_aerosol_model.cpp | New integration test demonstrating external aerosol model usage with two stub implementations |
| test/integration/CMakeLists.txt | Added aerosol_model test to build configuration |
Comments suppressed due to low confidence (1)
include/micm/system/system.hpp:38
- The class documentation still mentions "other associated phases" but the phases_ member has been removed. Update the documentation to accurately reflect that the class now contains only the gas phase and external models, not arbitrary additional phases.
/// @brief Represents the complete chemical state of a grid cell
/// Includes the gas phase and other associated phases, each with their own set of species.
class System
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #910 +/- ##
==========================================
+ Coverage 94.91% 94.95% +0.03%
==========================================
Files 64 64
Lines 3027 3030 +3
==========================================
+ Hits 2873 2877 +4
+ Misses 154 153 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR adds a minimally functional ability to integrate external modes (typically for clouds or aerosols) to MICM solvers. External models have the ability to contribute variables to the MICM state. These can include instances of phases and/or custom parameters (like particle number concentrations).
No processes are able to operate on state variable elements from external models in this initial implementation.
Also:
phases_andother_members of theSystemclass (not currently needed)I'm not sure that this current approach storing function pointers for external model functions is the best long term solution. But, I think it could be useful during development of these capabilities as it doesn't involve adding template parameters to the
Systemclass (which would require many updates throughout the code). After external models are fully supported, we could update theSystemclass to be templated if it makes sense.