External Model State Updates#924
Conversation
There was a problem hiding this comment.
Pull request overview
Adds square-bracket (operator[]) access to micm::State variables to support convenient external-model state reads/updates by variable index, unique name, or Species.
Changes:
- Introduced
State::operator[]overloads returning proxy objects for scalar and multi-gridcell variable access/mutation. - Added unit tests covering proxy assignment, const access, and arithmetic operations.
- Expanded aerosol-model integration tests to validate updating gas/external-model variables via the new
operator[]API (including multi-cell cases).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
include/micm/solver/state.hpp |
Declares operator[] overloads and proxy types for state variable access. |
include/micm/solver/state.inl |
Implements proxy behavior (assignment, indexing, arithmetic, comparisons) and operator[] lookups. |
test/unit/solver/test_state.cpp |
Adds unit tests for State::operator[] behavior in single- and multi-cell states. |
test/integration/test_aerosol_model.cpp |
Adds integration tests demonstrating external-model state updates via operator[] and refactors system setup into a helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #924 +/- ##
==========================================
- Coverage 94.63% 94.29% -0.34%
==========================================
Files 68 68
Lines 3185 3263 +78
==========================================
+ Hits 3014 3077 +63
- Misses 171 186 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| class VariableProxy | ||
| { | ||
| State& state_; | ||
| std::size_t index_; | ||
|
|
||
| public: | ||
| VariableProxy(State& state, std::size_t index) | ||
| : state_(state), | ||
| index_(index) | ||
| { | ||
| } | ||
|
|
There was a problem hiding this comment.
If this PR allows state to be manipulated by square bracket, it would effectively makes the usage of SetConcentration function obsolete. Is this a valid concern? Also I just noticed that State is struct but has public keywords. Should we consider converting State into a class and explicitly defining public/private members?
There was a problem hiding this comment.
We rely on SetConcentration in the wasm layer. Open to updating that if this is better though
There was a problem hiding this comment.
what if we make SetConcentration deprecated, and then we can remove it as a separate issue at some point? I could also modify this PR to just overload the SetConcetration function, but this seemed a little cleaner to me.
There was a problem hiding this comment.
Either way is fine with me
There was a problem hiding this comment.
I like the first approach better, since it lets us evaluate the impact of removing this function on tests/tutorials, as well as on upstream including musica, atmospheric physics and cam-sima (assuming we still care about cam-sima)
There was a problem hiding this comment.
sounds good! I'll add a deprecated comment to the SetConcentration fundtions
There was a problem hiding this comment.
updated! I like this approach too, it will let the other repos be gradually updated before removing the SetConcentration functions.
I left the SetConcentration() function that takes a map of species to concentrations in place. This wouldn't be covered by the square bracket functions.
boulderdaze
left a comment
There was a problem hiding this comment.
Thank you for addressing the comments!
This PR adds the ability for users to access and update the portion of the solver state associated with an external model (e.g., aerosol or cloud models).
Adds square bracket operators to the
Stateclass that can be used to set elements of theState.variables_matrix by variable index or unique name.