Skip to content

Comments

Improve unit conversion error logging (NGWPC-7604)#35

Merged
cmaynard-ngwpc merged 17 commits intorelease-candidatefrom
PhilMiller-7614-unit-conversion-logging
Sep 16, 2025
Merged

Improve unit conversion error logging (NGWPC-7604)#35
cmaynard-ngwpc merged 17 commits intorelease-candidatefrom
PhilMiller-7614-unit-conversion-logging

Conversation

@PhilMiller
Copy link

@PhilMiller PhilMiller commented Sep 11, 2025

When there are failures to convert units between data providers and BMI model consumers, we want to be able to track down the issue quickly and efficiently. We also don't want to be flooded with repetitive warnings about the same failed conversion.

Additions

  • Exception type representing the provider's failure to perform a requested conversion
  • Set of conversion failures seen previously, specific to the pair of formulations, variables, and units involved

Changes

  • Catch conversion failures in Bmi_Module_Formulation::get_value[s]() et al., add information, and rethrow rather than logging here
  • Catch the enriched exception in Bmi_Module_Formulation::set_model_inputs_prior_to_update(), check whether that variety of failure has already been reported, and log if it has not.

Removals

  • Logging in get_value which is now redundant with logging in the caller
  • Non-specific conversion failure logging flags

Todos

  • Currently unused get_values() call path isn't modified

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

@PhilMiller PhilMiller marked this pull request as ready for review September 12, 2025 20:03
@PhilMiller PhilMiller marked this pull request as draft September 12, 2025 20:18
@PhilMiller
Copy link
Author

The unit test failure is real - the change as it is right now breaks the tests for Bmi_Multi_Formulation

@PhilMiller PhilMiller changed the base branch from development to release-candidate September 12, 2025 21:25
@PhilMiller PhilMiller force-pushed the PhilMiller-7614-unit-conversion-logging branch from abe43ad to 30c77ce Compare September 15, 2025 23:36
@PhilMiller PhilMiller force-pushed the PhilMiller-7614-unit-conversion-logging branch from 30c77ce to b5c8543 Compare September 15, 2025 23:44
@PhilMiller PhilMiller changed the title Improve unit conversion error logging Improve unit conversion error logging (NGWPC-7604) Sep 16, 2025
@PhilMiller PhilMiller marked this pull request as ready for review September 16, 2025 06:08
Copy link

@cmaynard-ngwpc cmaynard-ngwpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed latest commits. Looks good from what I can tell.

@cmaynard-ngwpc cmaynard-ngwpc merged commit 41763b2 into release-candidate Sep 16, 2025
9 of 27 checks passed
@cmaynard-ngwpc cmaynard-ngwpc deleted the PhilMiller-7614-unit-conversion-logging branch September 16, 2025 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants