Skip to content

Conversation

@mattldawson
Copy link
Collaborator

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:

  • removes the phases_ and other_ members of the System class (not currently needed)
  • allows solvers to be created without processes (mostly for testing)

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 System class (which would require many updates throughout the code). After external models are fully supported, we could update the System class to be templated if it makes sense.

Copy link
Contributor

Copilot AI left a 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 ExternalModel struct to wrap external models via function pointers
  • Modified Phase::UniqueNames() to include phase name prefix and added SpeciesNames() for backward compatibility
  • Removed phases_ and others_ members from System class and related validation logic
  • Removed requirement for solvers to have processes (deleted valid_reactions_ flag and MissingProcesses error 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-commenter
Copy link

codecov-commenter commented Jan 30, 2026

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.95%. Comparing base (17a9afd) to head (2bbeae3).

Files with missing lines Patch % Lines
include/micm/system/system.hpp 87.50% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mattldawson and others added 7 commits January 30, 2026 15:25
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>
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.

3 participants