Skip to content

Add return_indices to local_thickness and porosimetry for compact output - #1164

Draft
ma-sadeghi wants to merge 2 commits into
devfrom
fix/840-compact-local-thickness-output
Draft

Add return_indices to local_thickness and porosimetry for compact output#1164
ma-sadeghi wants to merge 2 commits into
devfrom
fix/840-compact-local-thickness-output

Conversation

@ma-sadeghi

Copy link
Copy Markdown
Member

Closes #840.

The default float64 output of local_thickness/porosimetry becomes the peak-memory bottleneck on large tomograms (~21 GB for a 1500x1500x1200 volume in OP's case). With return_indices=True these now return (sizes, indices) where indices is uint8/uint16 and sizes[indices] recovers the float result. Output memory drops 8x or more.

To save memory inside porosimetry too (not just at the API boundary), drainage_dt gained a matching return_indices flag that skips allocating the float im_size array and writes the step index into a compact im_seq. The mode is opt-in and not compatible with outlets, since trapping post-processing relies on im_size.

Only method='dt' is supported for now; the other methods raise NotImplementedError. Easy to extend to conv/bf in a follow-up if needed.

# Before: 21 GB float64 output
lt = ps.filters.local_thickness(im, sizes=25)

# After: ~3 GB uint8 output, sizes[indices] recovers the float
sizes, indices = ps.filters.local_thickness(im, sizes=25, return_indices=True)

Default behaviour is unchanged. Two new tests added; full unit suite green.

…drainage_dt`

The default float64 output of `local_thickness`/`porosimetry` becomes the
peak-memory bottleneck on large tomograms (e.g. 21 GB for a 1500x1500x1200
volume). With `return_indices=True` these return `(sizes, indices)` instead,
where `indices` is a `uint8`/`uint16` image and `sizes[indices]` recovers
the float result. Memory drops 8x or more on the output.

To save memory inside `porosimetry` too (not just the returned array), the
underlying `drainage_dt` gained a matching `return_indices` flag that skips
allocating the float `im_size` array and writes the step index into a
compact `im_seq`. The mode is opt-in and not compatible with `outlets`,
since trapping post-processing relies on `im_size`.

Only `method='dt'` is supported for the new flag; the other methods raise
`NotImplementedError`. Closes #840.
@codecov

codecov Bot commented Apr 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.61538% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.5%. Comparing base (c352d60) to head (5c6468c).
⚠️ Report is 62 commits behind head on dev.

Additional details and impacted files
@@          Coverage Diff          @@
##             dev   #1164   +/-   ##
=====================================
  Coverage   70.4%   70.5%           
=====================================
  Files         45      45           
  Lines       6460    6488   +28     
=====================================
+ Hits        4554    4576   +22     
- Misses      1906    1912    +6     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ma-sadeghi

Copy link
Copy Markdown
Member Author

The test_bond_number failure on py3.13 ubuntu is the same flake fixed in #1161 — it'll clear once that lands and this branch is rebased. Everything else is green.

@ma-sadeghi
ma-sadeghi requested a review from jgostick April 28, 2026 13:21
@jgostick

jgostick commented May 5, 2026

Copy link
Copy Markdown
Member

I wonder if this is the right way to fix this? The reality is that all spheres drawn in a digital image have integer radii, so writing fractional radii is actually an artifact of the way the sizes are selected. If sizes is given as N, then it pulls out N sizes between 1 and max(dt). We could change the default behavior of the size generator, and then make the output (im_sizes) respect the dtype of the sizes? (i.e. int32)

@jgostick
jgostick marked this pull request as draft May 5, 2026 19:27
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.

Lower memory footprint for porosimetry and local_thickness

2 participants