Fixing digs

[spiani]

No description provided.

[Copilot]

Pull request overview

This PR replaces the previous “sequence_side/apply_dig” river digging utilities with a new Dig class and related APIs, adds support for “infinite” movements in configuration, and updates tests and actions to use the new implementation. It standardizes indexing to (latitude, longitude), introduces stricter bounds handling, and provides clearer test coverage for dig behavior.

• Introduces Dig, Direction, Movement, StartIndexStrategy, and DigOutsideShape with path slicing and mask filling.
• Refactors river digging action to use Dig for computing cells and sources, and to fill bathymetry directly.
• Adds tests for dig behavior (inside/outside domain, sources, start index strategies) and updates config to use infinite zonal movement.

Reviewed changes

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

File	Description
tests/test_dig.py	New tests covering dig path filling, handling outside-domain digs, source cell computation, and start index strategies to validate Dig’s behavior.
src/bathytools/utilities/dig.py	New Dig implementation with Movement parsing (including infinite “*”), start index strategies, bounds checks, path-to-slices conversion, mask filling, and source extraction; replaces legacy helpers.
src/bathytools/actions/dig_rivers.py	Refactors river digging to use Dig APIs, fixes index ordering to (lat_index, lon_index), and fills bathymetry via Dig; improves logging and error handling.
mer_domains/rivers/main.json	Updates river stem to use infinite zonal movement (“-*z”) for configuration-driven path extension.

ID: 001
Type: documentation
Severity: nit
File: src/bathytools/utilities/dig.py
Lines: 41-47
Comment: Direction.move_indices docstring mislabels indices: it says “i: Current x-coordinate/column index (represent latitude)” and “j: Current y-coordinate/row index (represent longitude)”. In this codebase, rows correspond to latitude and columns to longitude, and the implementation correctly moves EAST by increasing j (column) and NORTH by increasing i (row). The doc should instead state i is row (latitude) and j is column (longitude) to avoid confusion for callers.
Suggested fix: Update the docstring to:

• i: Current row index (latitude)
• j: Current column index (longitude)
  And adjust the earlier summary sentence accordingly.

ID: 002
Type: maintainability
Severity: nit
File: src/bathytools/utilities/dig.py
Lines: 317-326
Comment: Duplicate boundary check for j < 0: lines 317–326 check j < 0 twice (first at 317 and again within the compound condition at 321). While harmless, it’s redundant and makes the validation harder to read. Consider removing the repeated j < 0 inside the compound condition and keeping a single, clear check for each boundary.
Suggested change:

• Keep the separate if j < 0: ... block.
• Change the compound check to only if j + self.thick - 1 >= shape[1]: ....

ID: 003
Type: bug
Severity: moderate
File: src/bathytools/utilities/dig.py
Lines: 361-375
Comment: Potentially incorrect length for infinite movements when starting beyond the computed reaching point: for movement.length == -1, the code sets reaching_point = domain_shape[indx] - self.thick (or 0 for WEST/SOUTH) and then uses abs(reaching_point - starting_point) without adjusting direction. If starting_point > reaching_point (e.g., starting near/outside the far boundary and allow_outside=True), the computed positive length will move further outward instead of back toward the boundary. This can produce slices that extend away from the domain edge unintentionally.
Suggested fix: compute the signed delta based on the movement direction and clamp at zero:

• For EAST/NORTH: delta = max(0, reaching_point - starting_point)
• For WEST/SOUTH: delta = max(0, starting_point - reaching_point)
  Then use movement.copy(length=delta). This ensures the infinite movement extends toward the domain boundary and not away from it.

ID: 004
Type: documentation
Severity: nit
File: src/bathytools/utilities/dig.py
Lines: 207-213
Comment: Minor naming inconsistency in StartIndexStrategy docstring: it introduces “LEFT_TOP” in the description, but the enum values are BOTTOM_LEFT, CENTERED, and SIDE_CENTERED. This can confuse readers. Please align the description to the actual enum names (e.g., explain BOTTOM_LEFT rather than LEFT_TOP).

Comments suppressed due to low confidence (1)

src/bathytools/utilities/dig.py:321

• Test is always false, because of this condition.

        if j < 0 or j + self.thick - 1 >= shape[1]:

---

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

[Copilot]

Direction.move_indices docstring mislabels indices: it says “i: Current x-coordinate/column index (represent latitude)” and “j: Current y-coordinate/row index (represent longitude)”. In this codebase, rows correspond to latitude and columns to longitude, and the implementation correctly moves EAST by increasing j (column) and NORTH by increasing i (row). The doc should instead state i is row (latitude) and j is column (longitude) to avoid confusion for callers.

Suggested fix: Update the docstring to:

• i: Current row index (latitude)
• j: Current column index (longitude)
  And adjust the earlier summary sentence accordingly.

[Copilot]

@spiani I've opened a new pull request, #66, to work on those changes. Once the pull request is ready, I'll request review from you.

[Copilot]

@spiani I've opened a new pull request, #67, to work on those changes. Once the pull request is ready, I'll request review from you.