Perform basic sanity check on moment magnitude calculations#63
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds a validation check to moment_to_magnitude to raise a ValueError if the calculated magnitude exceeds 10.0, preventing issues when input moments are mistakenly provided in dyne-cm instead of Nm. A corresponding unit test is also added. The review feedback suggests adding a check for non-positive moments to avoid runtime warnings and invalid values, and addresses minor PEP 8 formatting issues regarding extra spaces and excessive blank lines.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a basic plausibility guard to moment_to_magnitude so obviously mis-scaled seismic moments (notably dyne-cm passed where N·m is expected) fail fast instead of producing unrealistic magnitudes.
Changes:
- Compute magnitude first, then raise a
ValueErrorwhen the resulting magnitude exceeds 10.0 (heuristic for “physically implausible”). - Add a unit-focused test ensuring dyne-cm-scale inputs trigger the new error.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/test_moment.py |
Adds a regression test asserting dyne-cm moments raise a ValueError. |
source_modelling/moment.py |
Introduces a magnitude upper-bound sanity check and returns computed magnitude after validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
AndrewRidden-Harper
left a comment
There was a problem hiding this comment.
Looks good. I explicitly checked the magnitude conversion maths
Checks if magnitude calculations are too large. If user provides dyne-cm then magnitudes are ~4.5 log-units higher than they should be because the calculation only works with Newton-metres.
Also refactored the magnitude scaling dispatch functions so that they now correctly convert magnitude types between Mw/BoldM where required (mostly, Leonard).