Skip to content

GEOPY-2910: Reduce chunking of sensitivities for TEM inversions - #170

Merged
domfournier merged 16 commits into
developfrom
GEOPY-2910B
Aug 24, 2026
Merged

GEOPY-2910: Reduce chunking of sensitivities for TEM inversions#170
domfournier merged 16 commits into
developfrom
GEOPY-2910B

Conversation

@domfournier

@domfournier domfournier commented Aug 18, 2026

Copy link
Copy Markdown

GEOPY-2910 - Reduce chunking of sensitivities for TEM inversions

follow up on fromer #150 and #152 that got reverted

Copilot AI lite review requested due to automatic review settings August 18, 2026 18:40
@github-actions github-actions Bot changed the title GEOPY-2910 b GEOPY-2910: b Aug 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR targets GEOPY-2910 by adjusting how sensitivity (Jacobian) blocks are chunked/computed in Dask-based simulations, aiming to reduce chunking overhead for TEM inversions.

Changes:

  • Refactors potential-fields sensitivity block computation and disk persistence to use Dask array/Zarr workflows.
  • Updates TEM time-domain sensitivity assembly to reduce per-chunk overhead by stacking/solving larger derivative blocks.
  • Adds an optimization flag to parallel block partitioning for TEM sensitivity computation.

Reviewed changes

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

File Description
simpeg/dask/potential_fields/base.py Reworks sensitivity block evaluation, distributed-client handling, and disk (zarr) persistence logic.
simpeg/dask/electromagnetics/time_domain/simulation.py Refactors derivative block handling and row assembly to reduce chunking overhead in TEM Jacobian computation.
Suppressed comments (3)

simpeg/dask/potential_fields/base.py:83

  • This client.submit(...) path should not be used when store_sensitivities == "disk", because the disk-writing code expects dask arrays (from_delayed) rather than distributed Futures. Gate this branch so disk mode always uses the delayed/from_delayed path.
    for count, block in enumerate(block_split):
        if client and worker:
            row = client.submit(

simpeg/dask/potential_fields/base.py:110

  • client.gather(rows) assumes rows is a list of Futures, but in disk mode the loop should be building dask arrays for to_zarr. If this branch runs in disk mode it will error or do the wrong thing; gate it the same way as the submit path.
    if client and worker:
        kernel = client.gather(rows)

simpeg/dask/electromagnetics/time_domain/simulation.py:481

  • When local_ind is empty, the function continues without advancing colm_count. That misaligns the column window (colm_count : colm_count + n_rec) for all subsequent receivers in the block, producing incorrect sensitivities. colm_count should advance by n_rec regardless of whether any data are kept for this receiver/time mask.
        if len(local_ind) < 1:
            row_block = np.zeros(
                (len(ind_array[1]), simulation.model.size), dtype=np.float32
            )
            rows.append(row_block)

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread simpeg/dask/potential_fields/base.py Outdated
Comment thread simpeg/dask/potential_fields/base.py Outdated
Comment thread simpeg/dask/potential_fields/base.py Outdated
@sebhmg sebhmg changed the title GEOPY-2910: b GEOPY-2910: Reduce chunking of sensitivities for TEM inversions Aug 24, 2026
@sebhmg
sebhmg requested a balanced review from Copilot August 24, 2026 15:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

simpeg/dask/electromagnetics/time_domain/simulation.py:482

  • When this chunk has no active time columns, continue skips the new column-offset update. The next chunk then slices field_derivs from the skipped chunk's columns, producing incorrect TEM sensitivities whenever an inactive chunk precedes an active one. Advance colm_count before continuing.
        if len(local_ind) < 1:
            row_block = np.zeros(
                (len(ind_array[1]), simulation.model.size), dtype=np.float32
            )
            rows.append(row_block)
            continue

simpeg/dask/potential_fields/base.py:118

  • With an assigned worker, rows contains distributed Future objects even after they have been gathered into kernel. Passing those futures to dask.array.concatenate fails because they do not expose array dimensions/chunks, so disk-backed sensitivities break in the worker-pinned distributed path. Concatenate the gathered/lazy kernel selected above instead.
        j_matrix = array.concatenate(rows, axis=0)

Comment thread simpeg/dask/potential_fields/base.py Outdated
domfournier and others added 6 commits August 24, 2026 08:39
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>
@domfournier
domfournier merged commit cbb27a4 into develop Aug 24, 2026
13 checks passed
@domfournier
domfournier deleted the GEOPY-2910B branch August 24, 2026 22:39
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