Vectorize string input for Angle and SkyCoord - #20236
Open
veyron-kairo wants to merge 5 commits into
Open
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
Parsing an array of angle strings goes through the PLY parser one element at a time, which dominates the cost of reading a coordinate column. Add parse_angles(), which normalises the separators with numpy string operations and hands the result to numpy.loadtxt, so a whole array is parsed in a fixed number of passes. Anything it does not recognise is flagged for the existing per-element parser, so the result is unchanged. Signed-off-by: Shridhar Panigrahi <sridharpanigrahi2006@gmail.com>
Only for arrays of at least 32 elements, since below that the per-element parser is quicker and this leaves scalar behaviour untouched. Signed-off-by: Shridhar Panigrahi <sridharpanigrahi2006@gmail.com>
The array parser has to give the same answer as the existing one, so the tests check exactly that, for the spellings it recognises, for one it does not, and for out-of-range fields. Signed-off-by: Shridhar Panigrahi <sridharpanigrahi2006@gmail.com>
A declination column is full of strings like '+41 16 09', and stripping signs from anywhere in a field meant those were handed to the per-element parser instead of being parsed here, losing the speed-up on half the data. It also let '12-34' through to numpy, which raised its own error rather than falling back. Signed-off-by: Shridhar Panigrahi <sridharpanigrahi2006@gmail.com>
Copy link
Copy Markdown
Contributor
|
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.
|
Sorry, something went wrong.
Signed-off-by: Shridhar Panigrahi <sridharpanigrahi2006@gmail.com>
Copy link
Copy Markdown
Contributor
Author
|
@taldcroft @mhvk whenever you have time, please take a look. Thanks! |
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is the input side of #20131, and the counterpart to #20130 which does the output.
Building an Angle or SkyCoord from an array of strings parses them one at a time through PLY, which is most of the cost of reading a coordinate column. Following @mhvk's suggestion on the issue I went with numpy rather than C: parse_angles normalises the separators with np.strings and lets np.loadtxt tokenise the whole array, with anything it doesn't recognise handed back to the existing parser. Only arrays of 32 or more take that path, so scalars and small arrays are untouched.
Angle on a 200k column goes from 2.85 s to 0.12 s, SkyCoord on 200k pairs is about 20x, and reading a 200k row VizieR style catalogue off disk is 28.7x overall, since the parsing was about 99% of it. Peak memory for a million strings drops from 1737 MB to 774 MB.
Output is bit identical to parsing one at a time, as exact float equality rather than a tolerance, over 6160 generated cases, the real VizieR columns in our ascii test data, 50k SkyCoord pairs, and against fractions.Fraction. The tests pin that equivalence, and there's a changelog entry. np.strings.partition only exists from numpy 2.1, so that call goes through np.char.partition below it.
Fixes #20131