Perf: speed up LIKELY ramp fitting by avoiding unnecessary allocations - #580
Perf: speed up LIKELY ramp fitting by avoiding unnecessary allocations#580jberg5 wants to merge 1 commit into
Conversation
|
Hello, thanks for your contribution! Did you use any generative AI tools when making this PR? That includes if the @jberg5 account itself is an automated agent. Thank you! |
|
Hi @zacharyburnett ! I'm a human, but I use AI extensively, and did so here. I'm trying out GPT6, and I instructed it to profile an end-to-end fitting workflow to look for opportunities to speed things up. It spotted this one, but the initial proposed change diff was to leave the diffs2use signature unchanged and do the relevant row extraction inside. I saw the stale docstring reference to per-row inputs, and decided to rewrite it into basically what you see here today, where the caller is responsible for selecting the relevant row. Of course, GPT6 then reviewed the diff before I submitted it :) and I also had it write the benchmark script you see above, and I had it run that and similar benchmarking on an x86 box just in case the allocation performance was better/worse on my ARM macbook (it's not). I was working off more realistic inputs locally but needed |
|
I started a romancal regtest run here: @t-brandt , you were looking for likelihood ramp fitting performance improvements recently; here's a low-hanging one. |
|
I agree this is an excellent change. Depending on the opinions of others, we could get the same answer with |
|
thanks @t-brandt ! I like your suggestion. I think it's faster as well. Totally up to you guys - the version I have here was motivated by keeping the diff minimal, happy to go with whichever you prefer. |
This PR makes
determine_diffs2usemuch faster; previously it was a substantial proportion of likely ramp fit runtime. The prior implementation took the full diffs cube (nresultants-1, rows, columns) and then allocated a fullonesarray in that shape, and then threw away all but the relevant row. Doing this full-cube allocation once per row was very expensive!determine_diffs2useonly needs to operate on a single row anyway (interestingly the docstring seems to have been assuming the caller was passing just that row'sdiffsof shape (ngroups-1, ncols), but the implementation assumed the cube).On a synthetic 1 x 5 x 4096 x 4096 Roman WFI-sized ramp,
LIKELYfitting decreased from 34s to 15s on my M3 mac. Both runs report the same mean fitted rate. See a benchmark script below.`benchmark.py`
Tasks
docs/pageno-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see changelog readme for instructions)changes/<PR#>.breaking.rstnews fragment"git+https://github.com/<fork>/stcal@<branch>")jwstregression testromancalregression test