Uh oh!
There was an error while loading. Please reload this page.
GEOPY-2910: Reduce chunking of sensitivities for TEM inversions - #170
Conversation
This reverts commit 4f3fc71.
# Conflicts: # simpeg/dask/electromagnetics/time_domain/simulation.py
There was a problem hiding this comment.
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)assumesrowsis a list of Futures, but in disk mode the loop should be building dask arrays forto_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_indis empty, the functioncontinues without advancingcolm_count. That misaligns the column window (colm_count : colm_count + n_rec) for all subsequent receivers in the block, producing incorrect sensitivities.colm_countshould advance byn_recregardless 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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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,
continueskips the new column-offset update. The next chunk then slicesfield_derivsfrom the skipped chunk's columns, producing incorrect TEM sensitivities whenever an inactive chunk precedes an active one. Advancecolm_countbefore 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,
rowscontains distributedFutureobjects even after they have been gathered intokernel. Passing those futures todask.array.concatenatefails because they do not expose array dimensions/chunks, so disk-backed sensitivities break in the worker-pinned distributed path. Concatenate the gathered/lazykernelselected above instead.
j_matrix = array.concatenate(rows, axis=0)
Uh oh!
There was an error while loading. Please reload this page.
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>
Uh oh!
There was an error while loading. Please reload this page.
GEOPY-2910 - Reduce chunking of sensitivities for TEM inversions
follow up on fromer #150 and #152 that got reverted