Allow vector-like string input in Quantity parsing - #19214

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.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

Hey @nstarman , I made some changes just like you suggested. Thank you very much for guiding me! 😉

Some timing stats:

Scalar String Parsing:

• main: 5.33 μs ± 311 ns
• this branch: 5.64 μs ± 82.4 ns

List-like inputs (u.Quantity([4, 8 , 9])) (no regression):

• main: 3.28 μs ± 235 ns
• this branch: 3.04 μs ± 55.3 ns

New Vector-like strings:

• this branch: 6.44 μs ± 151 ns

All timings are mean ± std over 7 runs, 100k loops each.

Apart from that, I have also tried to make regex a little bit different (using NUMS and VECTOR), I wonder if that's good....

Thanks @kartavayv! IMO, since scalar string input to a Quantity isn't meant to be in a hot loop, the minor performance regression is fine. Moreover, since this now enables a string vector input to Quantity, people can actually get a performance increase by moving the Quantity(str) out of the loop!

If we go with replacing white space with ",", we could use ast.literal_eval to do some of the parsing.

Thanks! @nstarman and @mhvk

Also, I think one might as well be generous on input and also accept commas (like [1., 2., 3.], as one gets in the repr).

I feel like we should do that, people might use commas (,) by habit, a lot, and it's a bit odd to allow something like "[4 7 8]" without commas....

However, should it not work recursively, so that it can deal with multi-dimensional quantities too?

This also looks like a good idea. I am working on it (interesting challenge!)

I will update shortly after making the suggested improvements!

Also, I was thinking, since the current code allows one dot, plus or minus [.+-] after a scalar and if i put NUM in VECTOR, then in VECTOR too, then v = re.match(pattern, value) selects, these characters after the allowed patterns too, which upon using, float() gives us : TypeError

Example

u.Quantity("56..")

OUTPUT: TypeError: Cannot parse "56.." as a Quantity. It does not start with a number.

Because ```v.group()`` is 56.. (dot-dot)

I think it's a good idea to make some changes in the pipeline to remove these extra characters, should I go in that direction?...or should I remove, [.+-]? entirely, which makes it a little bit strict (don't feel like it's a good idea) and creates a problem in unit_string creation

Start simple and build from there :). I recommend implementing changes as discrete commits, which makes building / reviewing changes much simpler.

@kartavayv - the inclusion of some of the extra stuff was on purpose, so that float could fail on a case where it is pretty obvious someone is trying to put in a number, and then give the hopefully more informative error message... But if it is easier as part of that to change it, go ahead (ideally ensuring that the tests keen on passing, i.e., that the error does not change).

IMPROVEMENTS

• Added new and better tests for string vectors with and without dimensions

• Added an extra strip() in data = data.replace(",", " ").strip().strip("[]").split() because it was trying to convert "9]" (for generic examples) into a float, which is weird, and I have no idea why that was happening! (Now it works fine!)

• Included spaces again as separators, which I had excluded (but I think "maintainers" like it)

NEW FEATURES...?

• Since @mhvk suggested the idea of multi-dimensional quantities, should I proceed to do that....?

SOMETHING UNUSUAL

Since now the parser accepts both spaces and commas as separators, so things like u.Quantity([7, 8 9]) also gives us nice Quantities, is that a good idea...?

@nstarman - while trying to remove mixed separators, I found out that whitespace should either work for grammar (parsing) or as a formatter.

CONTEXT:

Code snippet:

VECTOR: Final = rf"""
        \[\s*
        {NUM}
        (?:
        **((\s*,\s*)|
        (\s*))**  #subject to change
        {NUM}
        )*
        \s*\]
"""

We want a same separator for the whole input to prevent mixed separators. If we allow multiple white spaces as separators ( ((\s*,\s*)| (\s*)) --> (?P<sep>,|\s+) and we get an input with inconsistent number of white spaces between the numbers, the regex backtracks and uses \s*+ from \s*\] to parse the inconsistent whitespaces (i.e. (?P<sep>,|\s+) fails as soon as it encounters an inconsistent whitespace between the numbers). Same is the case with using a single space ((?P<sep>,| )) as a separator, in case of multiple whitespaces (?P<sep>,|\s+) fails, and if we use \s*(?P<sep>,|\s+)\s*, then first \s* consumes all the whitespaces, and (?P<sep>,|\s+) still fails to capture a space as a separator.

But regex still parses the string in both the cases mentioned above, because of \s*\], which means each digit is parsing independently because of regex backtracking.

So, either we need to use whitespaces as only separators or to do just formatting (i.e. allow any number of spaces between the [] and numbers.

I guess using only commas as separator and allowing any number of white spaces before and after it is a good idea.

I might be missing something, any design suggestions will help a lot 😉

@kartavayv the regex can distinguish between space between [ x / x ] where it's for formatting versus between numbers [x y] where it's the delimiter. So we should be able to support [ x y ]/[x y ]/etc. versus [ x, y ].

I was trying to add an indefinite number of white spaces as a delimiter 😢🫤, now it's fine!

Should I also try to add multi-dim vector like strings or tackle that in some other PR...?

Thanks a lot for helping me ❤️

@kartavayv - sorry to comment without actually having had time to look at the PR again, but my tendency would be to try to do the multi-D as well. I think you could do it by iteratively interpreting strings inside [...]. But it is probably best to first experiment outside of astropy on how this would work, and then push it up (I recommend generally to test locally, as waiting for CI to run is slow and a bit wasteful of resources). That said, I'm not stuck to having multi-D work in the first round.

@mhvk this PR came about from needing to read 1D Quantity strings in #19110. For the sake of that PR, I'd prefer to keep this PR on the nice expansion from scalars to 1D arrays and leave the 1D -> ND generalization for a followup.

Sure, fine to do it in steps!

@nstarman - I tried the edge cases that you suggested, and they are getting parsed 😊. Probably you are pointing to something else that I am unable to see yet!

1. Initially, I thought to allow only a single space and single comma as a separator (which is strict), but since you are now suggesting an indefinite number of spaces in between the numbers, I am going to do that! Also, if there are n spaces between two numbers, we won't make it strict to have the same number of spaces between any other two adjacent numbers, which allows vectors like:
   [ 7 8 9] and with units [ 7 8 9] eV

   So, zero expectations of consistent separator behavior

2. I tried using SEP = r"(?:\s*,\s*|\s+)" but it is accepting mixed separator, then I tried using capturing groups but that's getting way to complex and since you suggested this, I have separated VECTOR_1D into 2 sort of subclasses VECTOR_COMMA and VECTOR_SPACE (although the names are bad!). This prevents mixed separators, like [7, 8 9]

3. Acc. to your suggestion, I have made changes to allow trailing separators, so things like "[1,2,3,]" will get parsed now!

Just to be clear, my last commit implements all my comments...