Update Error helper functions by boulderdaze · Pull Request #864 · NCAR/musica

boulderdaze · 2026-04-29T01:09:41Z

In progress

This PR:

Reorganizes the MUSICA utility files by spliting into separate categories: error, string, and util
- utils.hpp contains: String, Error, Configuration, Mapping/Mappings, IndexMapping/IndexMappings
Removes void ToError(const char* category, int code, int severity, Error* error); because the 3 argument overload is only defined but never called.
Removes DeleteIndexMapping that never needs individual clean up (contains size_t and double)
Fixes dangling pointers in DeleteMappings and DeleteIndexMappings
Update the organization of Errors.
Temporarily disables FetchContent GitHub Actions workflow due to a linking issue.
- Specifically, MUSICA was compiled on Fedora using GCC (libstdc++), but later linked in an Ubuntu environment with IntelLLVM (icpx). Because these environments rely on different C++ standard libraries/ABIs, required symbols from the GCC-built library aren’t resolved during linking. Issue Rewrite the CMake configuration to avoid FetchContent for MUSICA #869 was created to address this.

codecov-commenter · 2026-04-29T01:15:05Z

Codecov Report

❌ Patch coverage is 42.03822% with 91 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.03%. Comparing base (44e056d) to head (9caba50).

Files with missing lines	Patch %	Lines
src/utils/error_code.cpp	0.00%	34 Missing ⚠️
src/utils/error_handler.cpp	68.75%	10 Missing ⚠️
src/micm/cpu_solver.cpp	11.11%	8 Missing ⚠️
src/tuvx/grid_map.cpp	0.00%	6 Missing ⚠️
src/tuvx/profile_map.cpp	0.00%	6 Missing ⚠️
src/tuvx/radiator_map.cpp	0.00%	6 Missing ⚠️
src/micm/v1_parse.cpp	0.00%	3 Missing ⚠️
src/tuvx/tuvx.cpp	0.00%	3 Missing ⚠️
src/utils/util.cpp	84.21%	3 Missing ⚠️
include/musica/utils/error_handler.hpp	75.00%	2 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #864      +/-   ##
==========================================
- Coverage   73.18%   73.03%   -0.16%     
==========================================
  Files         127      132       +5     
  Lines       10219    10228       +9     
==========================================
- Hits         7479     7470       -9     
- Misses       2740     2758      +18

Flag	Coverage Δ
cpp_fortran	`63.78% <42.03%> (-0.28%)`	⬇️
javascript	`93.22% <ø> (ø)`
python	`79.15% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR refactors MUSICA’s shared utility/error layer by splitting the old utility header/source into separate utils components, introducing typed error-code/exception helpers, and updating MICM/TUV-x/bindings/tests to the new layout. It also adjusts CI by moving the FetchContent workflow out of the active workflow set.

Changes:

Split string/error/util functionality into new include/musica/utils/* and src/utils/* files.
Switched many MICM/TUV-x error paths to the new error-code/exception plumbing and updated tests/bindings to match new categories/includes.
Removed the active FetchContent CI job from docker.yml and added a disabled standalone workflow file.

Reviewed changes

Copilot reviewed 53 out of 53 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/utils/util.cpp	Keeps config/mapping utilities after split
src/utils/string.cpp	New string helper implementation
src/utils/error_handler.cpp	New error helper implementation
src/utils/error_code.cpp	New error-category definitions
src/tuvx/tuvx.cpp	Broadens TUV-x exception catches
src/tuvx/tuvx_c_interface.cpp	Broadens C interface exception catch
src/tuvx/radiator.cpp	Broadens radiator exception catches
src/tuvx/radiator_map.cpp	Broadens radiator-map exception catches
src/tuvx/profile.cpp	Broadens profile exception catches
src/tuvx/profile_map.cpp	Broadens profile-map exception catches
src/tuvx/grid.cpp	Broadens grid exception catch
src/tuvx/grid_map.cpp	Broadens grid-map exception catches
src/test/unit/util.cpp	Updates util/error assertions
src/test/unit/micm/micm_c_api.cpp	Updates MICM error assertions
src/micm/v1_parse.cpp	Switches parser throws to `musica::Exception`
src/micm/v0_parse.cpp	Switches parser throws to `musica::Exception`
src/micm/state.cpp	Updates util include path
src/micm/state_c_interface.cpp	Removes local error wrapper, updates codes
src/micm/parse.cpp	Uses new parse error codes
src/micm/micm.cpp	Uses new MICM error codes
src/micm/micm_c_interface.cpp	Uses shared `HandleErrors` helper
src/micm/cpu_solver.cpp	Uses new exception/error-code types
src/micm/convert_v0_to_v1.cpp	Uses new parse exception type
src/cuda/cuda_solver.cpp	Updates util include path
src/CMakeLists.txt	Repoints common utility sources
python/test/unit/test_error_handling.py	Updates referenced error header path
python/bindings/tuvx/tuvx.cpp	Updates util include path
python/bindings/error.hpp	Updates util include path
python/bindings/error.cpp	Updates error header path
javascript/bindings/musica_wasm.cpp	Updates util include path
include/musica/utils/util.hpp	New split utility public header
include/musica/utils/string.hpp	New public string header
include/musica/utils/error.hpp	New public error macro header
include/musica/utils/error_handler.hpp	New public error helper header
include/musica/utils/error_code.hpp	New public error-code header
include/musica/tuvx/tuvx.hpp	Updates util include path
include/musica/tuvx/radiator.hpp	Updates util include path
include/musica/tuvx/radiator_map.hpp	Updates util include path
include/musica/tuvx/profile.hpp	Updates util include path
include/musica/tuvx/profile_map.hpp	Updates util include path
include/musica/tuvx/grid.hpp	Updates util include path
include/musica/tuvx/grid_map.hpp	Updates util include path
include/musica/micm/state_c_interface.hpp	Updates util include path
include/musica/micm/solver_interface.hpp	Updates exception docs/includes
include/musica/micm/parse.hpp	Removes legacy parse error machinery
include/musica/micm/micm.hpp	Removes legacy MICM error machinery
include/musica/micm/micm_c_interface.hpp	Updates util include path
include/musica/error.hpp	Deletes legacy public error header
fortran/util.F90	Updates included error header path
fortran/test/unit/util.F90	Updates included error header path
fortran/test/integration/test_micm_api.F90	Updates parse error macro names
.github/workflows/fetch-content.yml.disabled	Disabled standalone FetchContent workflow
.github/workflows/docker.yml	Removes active FetchContent job

Comments suppressed due to low confidence (2)

include/musica/utils/util.hpp:7

The package no longer installs the old <musica/util.hpp> entry point or the IsError(...) helper that downstream code used from it. That turns this refactor into a source-breaking public API change for external consumers unless you keep a compatibility shim.
src/utils/util.cpp:95
The new configuration.data_ == nullptr early-return path is not exercised by the existing util tests, even though this file already has good CreateIndexMappings coverage. Please add a unit test for calling CreateIndexMappings with a default/cleared Configuration so this new error contract does not regress silently.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

K20shores

Beautiful PR. I feel like this is at least slightly simpler than what we were doing before, and maybe even less error prone. Couple of questions though

K20shores · 2026-05-06T03:04:13Z

+  /// @brief Singleton category declarations
+  const std::error_category& musica_category();
+  const std::error_category& micm_category();
+  const std::error_category& parse_category();


Why do we need singletons for these? We shouldn't do this if we don't need to. musica could potentially be called from multiple threads in a downstream model and the static initialization order could cause some issues. If we really need these, we probably need to throw a mutex in the constructors for these

K20shores · 2026-05-06T03:05:38Z

+  /// These are provided solely to support the short-form Exception constructor
+  template<typename T> const char* error_category_for();
+  template<> inline const char* error_category_for<ErrorCode>()      { return MUSICA_ERROR_CATEGORY; }
+  template<> inline const char* error_category_for<MicmErrorCode>()  { return MUSICA_MICM_ERROR_CATEGORY; }
+  template<> inline const char* error_category_for<ParseErrorCode>() { return MUSICA_PARSE_ERROR_CATEGORY; }


Seems like we only use error_category_for<ErrorCodeEnum>? Do we really need these?

boulderdaze added 6 commits April 27, 2026 17:17

reorganize util files

bb50c46

update the util path

144b5bc

rename to distiguish c/fortran api vs c++

62f5d62

error code

f79462e

update the error handler

9aef6a9

update the error

e999abb

boulderdaze added 6 commits April 28, 2026 19:22

code cleanup

acee62a

update the error

0511e0c

add directives

ef59b2f

Merge branch 'main' into 855-error-helper

3af437e

merge main and resolve conflicts

89f310b

disable fetch content action temporarily

a46af75

boulderdaze requested review from Copilot May 5, 2026 23:57

Copilot started reviewing on behalf of boulderdaze May 5, 2026 23:57 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

boulderdaze mentioned this pull request May 6, 2026

Rewrite the CMake configuration to avoid FetchContent for MUSICA #869

Open

boulderdaze self-assigned this May 6, 2026

boulderdaze added 2 commits May 5, 2026 19:49

address the comments

06ce26e

correct the test

9caba50

boulderdaze marked this pull request as ready for review May 6, 2026 02:23

boulderdaze requested a review from K20shores May 6, 2026 02:24

K20shores requested changes May 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Error helper functions#864

Update Error helper functions#864
boulderdaze wants to merge 14 commits intomainfrom
855-error-helper

boulderdaze commented Apr 29, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Apr 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

K20shores left a comment

Uh oh!

K20shores May 6, 2026

Uh oh!

K20shores May 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

boulderdaze commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

K20shores left a comment

Choose a reason for hiding this comment

Uh oh!

K20shores May 6, 2026

Choose a reason for hiding this comment

Uh oh!

K20shores May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

boulderdaze commented Apr 29, 2026 •

edited

Loading

codecov-commenter commented Apr 29, 2026 •

edited

Loading