Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Python API to store all grid cells in a single internal state instead of using multiple state objects. Previously, the code would split grid cells across multiple states based on the vector size, but this approach is being simplified to use a single state with explicit vector-aware indexing for the vector ordered matrix case.
Key Changes
- Removed multiple internal states in favor of a single state object
- Implemented explicit vector-aware indexing for concentrations, rate parameters, and conditions
- Updated vector size default from 1 to 4 for standard order solvers
- Added comprehensive test coverage for 6 grid cells to validate the refactoring
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| python/musica/micm/state.py | Core refactoring - replaced multiple states list with single state object and added vector-aware indexing logic |
| python/musica/micm/micm.py | Simplified solve method to use single state instead of iterating over multiple states |
| python/musica/bindings/micm/micm.cpp | Changed vector size for standard order solvers from 0 to 1 |
| pyproject.toml | Increased default MICM vector size from 1 to 4 |
| python/musica/test/unit/micm/test_state.py | Added extensive tests for 6 grid cells to validate new single-state approach |
| python/musica/test/integration/test_sulfate_box_model.py | Added pytest.main boilerplate for direct script execution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
📄 Documentation for this branch is available at: https://ncar.github.io/musica/branch/remove_multiplate_states/ |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #654 +/- ##
==========================================
+ Coverage 81.52% 81.55% +0.03%
==========================================
Files 114 114
Lines 9921 9912 -9
==========================================
- Hits 8088 8084 -4
+ Misses 1833 1828 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
K20shores
force-pushed
the
remove_multiplate_states
branch
from
7d3decf to
3967b53
Compare
K20shores
force-pushed
the
remove_multiplate_states
branch
from
3967b53 to
4633d9e
Compare
In conjunction with the work the students are doing in #642, I realized that we were not handling multiple grid cells in a single state like I thought we were.
This PR removes multiple states and stores all grid cells in a single state in the Python API. I think the reason we did what we did before was for the cuda solver. The cuda solver always expects the number of grid cells to equal the vector length, but this will be changing soon. For that reason, this PR can wait until those updates are made in micm (which should be part of the resolution of NCAR/micm#857).
To handle multiple grid cells, it's a special case specifically for the vector ordered matrix, where we need to ensure we properly index our data structures by explicitly taking into account the vector length, which we are now doing. It worked before because we would create as many internal states who only stored a vector lengths worth of grid cell data