Conversation
|
📄 Documentation for this branch is available at: https://ncar.github.io/musica/branch/541-wrap-carmagroup_get/ |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new function to retrieve group properties from CARMA, addressing issue #541. The implementation provides access to detailed group-specific parameters like bin radii, masses, optical properties, and element configurations through a new get_group method.
- Adds
GetGroupmethod to C++ CARMA class that retrieves comprehensive group properties - Implements Fortran interface function
internal_get_carma_parametersthat callsCARMAGROUP_Get - Exposes group property retrieval functionality through Python bindings
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/carma/interface.F90 | Adds Fortran interface function to retrieve group parameters via CARMAGROUP_Get |
| src/carma/carma.cpp | Implements GetGroup method and updates constructor to store both C++ and C-compatible parameters |
| musica/test/test_carma.py | Adds test call to verify get_group functionality |
| musica/carma.py | Implements Python get_group method and fixes line length formatting issues |
| musica/carma.cpp | Adds Python binding for _get_group method |
| include/musica/carma/carma_c_interface.hpp | Declares InternalGetGroupProperties C interface function |
| include/musica/carma/carma.hpp | Adds CARMAGroupProperties struct and GetGroup method declaration |
Comments suppressed due to low confidence (1)
musica/test/test_carma.py:63
- The test only prints the result but doesn't verify the returned group properties. Consider adding assertions to validate the structure and values of the returned group data.
print(carma.get_group(1))
src/carma/interface.F90
Outdated
| subroutine internal_get_carma_parameters(carma_cptr, group_index, nbin, nwav, nelem, bin_radius_ptr, bin_radius_lower_bound_ptr, & | ||
| bin_radius_upper_bound_ptr, bin_width_ptr, bin_mass_ptr, & | ||
| bin_width_mass_ptr, bin_volume_ptr, projected_area_ratio_ptr, & | ||
| radius_ratio_ptr, porusity_ratio_ptr, extinction_coefficient_ptr, & |
There was a problem hiding this comment.
The variable name 'porusity_ratio_ptr' contains a spelling error. It should be 'porosity_ratio_ptr' (porosity, not porusity).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #545 +/- ##
==========================================
- Coverage 81.14% 80.20% -0.94%
==========================================
Files 52 52
Lines 5679 5748 +69
==========================================
+ Hits 4608 4610 +2
- Misses 1071 1138 +67 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Closes #541
I didn't add a test because I don't know what values to get, except for calling it from python which makes sure it doesn't segfault and fail