-
Notifications
You must be signed in to change notification settings - Fork 9
Fix JavaScript state indexing for vector-ordered matrices #729
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
Conversation
Co-authored-by: K20shores <[email protected]>
Co-authored-by: K20shores <[email protected]>
Co-authored-by: K20shores <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #729 +/- ##
==========================================
- Coverage 76.63% 75.80% -0.83%
==========================================
Files 108 108
Lines 7849 7974 +125
==========================================
+ Hits 6015 6045 +30
- Misses 1834 1929 +95
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:
|
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 fixes JavaScript state indexing for vector-ordered matrices by refactoring the WASM bindings to exclusively use embind for data marshaling between JavaScript and C++. The implementation adds new APIs to the musica state/micm classes, consolidates the vector size function for reuse across wrappers, and extends support for any number of grid cells regardless of solver type.
Changes:
- Added
GetMusicaVectorSize()function in C++ for consistent vector size retrieval across Python and JavaScript - Implemented new
SetConcentrations(),GetConcentrations(),SetRateConstants(), andGetRateConstants()methods on State class that handle vector-ordered matrix indexing - Refactored JavaScript bindings to use embind exclusively, removing custom wrapper classes (StateWrapper, MICMWrapper)
- Expanded JavaScript tests to cover multiple grid cells (1, 5, 10, 20) and all solver types
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/micm/state.cpp | Added new methods for setting/getting concentrations and rate constants with proper vector-ordered indexing |
| src/micm/micm_c_interface.cpp | Implemented GetMusicaVectorSize() function for determining vector size based on solver type |
| src/micm/micm.cpp | Added convenience constructor accepting config path directly |
| python/bindings/micm/micm.cpp | Updated to use shared GetMusicaVectorSize() function |
| javascript/src/musica_wasm.cpp | Complete rewrite using embind exclusively, removed custom wrapper classes |
| javascript/src/micm/*.h/cpp | Deleted wrapper files, replaced with direct embind bindings |
| javascript/micm/*.js | Updated to use new WASM API with proper parameter naming (airDensities) |
| javascript/tests/**/*.test.js | Refactored tests to reduce duplication and test multiple grid cells/solver types |
| javascript/CMakeLists.txt | Removed wrapper source files from build |
| package.json | Added -DMUSICA_ENABLE_TESTS=OFF to prevent building tests during WASM build |
| include/musica/micm/*.hpp | Added new method declarations and forward declaration to break circular dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /// @brief Get the MUSICA vector size | ||
| /// @return The MUSICA vector size | ||
| std::size_t GetMusicaVectorSize(musica::MICMSolver); |
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.
The function name could be just GetVectorSize() to be consistent with other functions such as?
size_t GetMaximumNumberOfGridCells(MICM *micm);
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.
true
This PR does several things
Fixes #697