Repository navigation
Grow VOTable result arrays in place instead of copying them - #20743
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
baaee44 to
0e75634
Compare
|
👏
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. |
|
Oh, those are better than I expected on speed! I started this branch only worrying about the memory of that big Gaia table, but the speedups are nice, and the benchmarked small tables have modest memory gains, up to the size of the table. |
stvoutsin
left a comment
stvoutsin
left a comment
There was a problem hiding this comment.
Yea this makes total sense, we don't really need the masked array until the end.
A few comments added here, but no show-stoppers.
|
One general thing I'll add is that numpy is considering deprecating though there is some pushback for precisely our use case, and that it is also used internally within numpy. I think if it does eventually get deprecated, then we can use whatever the new, recommended interface is, but that will likely wipe out the gains here. One could also do this in C, and get the gains back. Maybe cross that bridge if/when we get to it. Regardless, I think there is value in this PR here, now as the Builder class puts all of this into one location and documents it, thanks to your suggestion @stvoutsin . |
- While reading a TABLEDATA, BINARY or BINARY2 table, keep the values and the missing-value flags in plain numpy arrays that can be resized in place, and make the masked array once at the end. Before, the whole table was copied each time the arrays filled up, and once more at the end to trim them. - Reading the 2.5 GB Gaia table from astropygh-8946 peaks at 5.1 GB instead of 6.8 GB, and takes 113s instead of 130s. That's a 25% reduction in memory and a 15% speed increase. - Result is the same: same type, dtype, mask, fill value and values. - Add a test reading a 1000-row table in each format, which grows the arrays past the first 512 rows and then trims them.
- Replace the two helper functions with _MaskedArrayBuilder, which owns the data and mask arrays, resizes them together and never hands them out, so no view of them can outlive a refcheck=False resize. The parsers set rows through set_row and set_rows. - Test reading a variable-length column longer than the first allocation, and tables whose nrows attribute is too small, correct or too large, in each format.
daf09d3 to
2b35b7a
Compare
|
I see an approval. Providing CI passes, is this good to merge? |
Thanks, that's useful to know! In any case, this all looks good to go from my pov! |
Do we need a reminder issue or you just want to YOLO this? :) |
|
YOLO! If Feel free to merge once the tests complete. The current test failures are the usual culprits from the weekly pytest API change and the interwebs being on fire. |
|
Green! Srsly, even the benchmarks are wacky. Unrelated coordinates and io_ascii performance decreases. |
|
Hmm benchmark report is a bit weird but maybe because your branch is not up-to-date with
|
|
Thanks, all! |
|
p.s. Wow I didn't know |
Description
This PR makes VOTable reading faster, and it uses less peak memory.
Reading a VOTable used to copy the whole table every time the result arrays filled up, and once more at the end to trim them. This was required because masked arrays were used and cannot grow in-place. This PR keeps the values and the missing-value mask in two plain numpy arrays that can be grown and trimmed in place, and builds the masked array once at the end.
Funny enough, this is not new. The reader worked this way until 2012, when 13d7400 put the values and the mask into a single masked array for a simpler interface (with the Table class), and growing the table began to copy it. That interface does not change here; only the reading goes back to two arrays. It is faster and more memory efficient to do so.
Reading the 2.5 GB Gaia table from #8946 (with #20509 merged) now peaks at 5.1 GB memory usage instead of 6.8 GB and takes 113s instead of 133s. That's a 25% improvement in peak memory usage (up the size of the table) and a 15% speedup.
The table that comes back is unchanged: same type, dtype, mask, fill value and values. A new test reads 1000 rows in each of TABLEDATA, BINARY and BINARY2, which grows the arrays past the first 512 rows and then trims them.
The downstream pyvo and astroquery test suites pass as well (locally), using the tox jobs from #20604.
AI Disclosure
I used Claude Opus 5.5 to identify where there was still unneeded memory usage in the VOTable reader, and to help draft the changes here.
Merge method