Skip to content

Fix minimum jumping depth - #80

Merged
lispandfound merged 15 commits into
mainfrom
jump_point_fix
Aug 24, 2026
Merged

Fix minimum jumping depth#80
lispandfound merged 15 commits into
mainfrom
jump_point_fix

Conversation

@lispandfound

Copy link
Copy Markdown
Contributor

Minimum jumping depth was not being respecting in the source modelling jump point calculator. This fixes it so it can used in Cybershake.

@lispandfound

Copy link
Copy Markdown
Contributor Author

@claude review this PR please.

@claude

claude Bot commented Aug 16, 2026

Copy link
Copy Markdown

Claude finished @lispandfound's task in 2m 35s —— View job


Review complete ✅

  • Gather context (diff, existing review comments)
  • Review source_modelling/rupture_propagation.py
  • Review source_modelling/sources.py
  • Review tests/test_sources.py
  • Post findings / summary

Findings: 1 medium, 1 low

  1. Mediumsource_modelling/rupture_propagation.py:527-530: jump_points_from_rupture_tree passes min_depth straight into sources.closest_points_beneath for every source/parent pair with no clamping. closest_points_beneath raises ValueError if min_depth exceeds the bottom depth of either source. The existing caller in moment.py (connected_components) clamps min_depth per-pair against 0.99 * bottom_m to avoid exactly this; the new code here doesn't, so a Cybershake rupture tree containing a fault/point shallower than the configured min_depth will raise and abort processing of the whole tree instead of gracefully skipping the depth restriction for that pair. No new test exercises this shallow-source case.
  2. Low (nit)source_modelling/rupture_propagation.py:527: elif min_depth: is a truthiness check rather than is not None, so an explicit min_depth=0.0 silently behaves like None. Harmless here since 0 restricts nothing, but worth tightening for clarity.

Everything else (the new Point.top_m/bottom_m properties, the IsSource type widening on closest_points_beneath, and the added test) looks consistent with the existing Plane/Fault conventions.

Comment thread source_modelling/rupture_propagation.py Outdated
Comment thread source_modelling/rupture_propagation.py Outdated

@AndrewRidden-Harper AndrewRidden-Harper left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some minor suggestions for comments

Comment thread source_modelling/rupture_propagation.py Outdated
Comment thread source_modelling/rupture_propagation.py
Comment thread source_modelling/sources.py
Co-authored-by: Andrew Ridden-Harper <52001209+AndrewRidden-Harper@users.noreply.github.com>

@AndrewRidden-Harper AndrewRidden-Harper left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, excellent comments!

@lispandfound
lispandfound merged commit 3a78866 into main Aug 24, 2026
7 checks passed
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