FIX: reduce memory in flatfield by evaluating tilts only at slit pixels#2098
FIX: reduce memory in flatfield by evaluating tilts only at slit pixels#2098tepickering wants to merge 2 commits intopypeit:developfrom
Conversation
Replace full-frame meshgrid tilt evaluation with per-slit-pixel evaluation using PypeItFit.eval directly. For spectrographs with many slits (e.g., fiber-fed IFUs with hundreds of fibers), the previous approach allocated a full-frame tilts array per slit, causing excessive memory usage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kbwestfall
left a comment
kbwestfall
left a comment
There was a problem hiding this comment.
I'm approving this as it is, given that it reduces memory so substantially. But I had some questions that I hope we can think through before we merge this.
pypeit/flatfield.py
Outdated
| # Build a full-frame tilts image placeholder with only slit pixels filled | ||
| tilts = np.zeros(rawflat.shape, dtype=float) | ||
| tilts[onslit_padded] = _tilts_slit | ||
| del _tilts_slit, _spec, _spat |
There was a problem hiding this comment.
3 questions:
- Instead of adding this here, could we instead pass
onslit_paddedtotracewave.fit2tiltsand essentially get the same thing? - Do we need to explicitly delete the "work" arrays, or can we lean on garbage collection?
- I'm wondering if there's a way we could minimize the number of times we need to create the
tiltsarray, and/or try to use the same memory block.
There was a problem hiding this comment.
3 questions:
- Instead of adding this here, could we instead pass
onslit_paddedtotracewave.fit2tiltsand essentially get the same thing?
maybe. i'll take a deeper look.
- Do we need to explicitly delete the "work" arrays, or can we lean on garbage collection?
i don't see any downside to just manually deleting them if we know they're not going to be used. especially given what look like GC issues with python 3.14.
- I'm wondering if there's a way we could minimize the number of times we need to create the
tiltsarray, and/or try to use the same memory block.
given the scale of the bug being fixed here, i am sure there are plenty of places in the code that can be streamlined and made more efficient.
There was a problem hiding this comment.
asked claude and got this response:
Could we pass onslit_padded to fit2tilts instead? — Yes, this is a clean approach. fit2tilts currently creates a full nspec x nspat meshgrid (line 893), which is the memory problem. Adding an optional mask parameter would let it evaluate only at masked pixels and return either a sparse full-frame array or just the 1D values. There are only 2 other callers (wavetilts.py:154 and wavetilts.py:817), so backward compatibility is easy with a default mask=None. This would also benefit those callers if they ever need it.
Explicit del vs garbage collection — The del statements are a belt-and-suspenders measure. In a tight loop over hundreds of slits, it ensures the previous iteration's arrays are freed before allocating the next. GC would eventually collect them, but in CPython the reference counting means del triggers immediate deallocation. Given the memory-sensitive context, it's cheap insurance.
Reuse the tilts array across iterations — Currently tilts = np.zeros(...) allocates a new array each iteration. You could allocate once before the loop and tilts[:] = 0 each iteration to reuse the memory block. However, spec_coo is derived from tilts on the next line, so both would need coordinated handling. Modest win for standard spectrographs, bigger win for many-slit cases.
claude's point about cpython is a good one since the objects in question are numpy arrays.
Add an optional `slit_mask` parameter to `tracewave.fit2tilts` so that tilt evaluation at only the relevant slit pixels is handled inside the function rather than being inlined at each call site. This addresses review feedback on the memory optimization: the logic now lives in the canonical location and both callers in `flatfield.py` and `wavetilts.py` benefit from reduced memory usage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
went ahead and implemented @kbwestfall's idea of doing the fix at the |
2 participants
Replace full-frame meshgrid tilt evaluation with per-slit-pixel evaluation using PypeItFit.eval directly. For spectrographs with many slits (e.g., fiber-fed IFUs with hundreds of fibers), the previous approach allocated a full-frame tilts array per slit, causing excessive memory usage.
This was originally implemented in #2080 and was required to get the original implementation there to work. However, it's a massive improvement for any multi-slit mode.