Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parsing of single digit angles #4

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

briochemc
Copy link

Fixes #2?

This PR is a WIP and adds parsing for strings with a single digit such as "1'W".

Since this is a WIP, this first commit only changed the degree parsing function parse_dms (not the hour parsing one, parse_hms).

This PR is breaking the current tests. In particular, my interpretation (maybe wrong, asked in the linked issue) is that the sign at the front should be broadcast to all the digits. Having only edited parse_dms and not parse_hms, we can see the difference in behavior:

julia> parse_dms("-1:2:3") # in this PR the sign is broadcasted
(-1.0, -2.0, -3.0)

julia> parse_hms("-1:2:3") # in the current version the sign is only applied to the first digit
(-1.0, 2.0, 3.0)

This PR essentially splits the regex for parsing in a regex for each component to allow to grab each individual item. For example, these now work as expected (IMHO):

julia> strings_to_test = ["1$sep$d" for sep in ["°", "'", "\""] for d in ["N", "S", "E", "W"]] ;

julia> expected_output = [(occursin(d,"NE") ? 1.0 : -1.0) .* (sep=="°" ? (1,0,0) : (sep=="'" ? (0,1,0) : (0,0,1))) for sep in ["°", "'", "''"] for d in ["N", "S", "E", "W"]] ;

julia> function myparse(str)
           try
               parse_dms(str)
           catch e
               "does not parse"
           end
       end
myparse (generic function with 1 method)

julia> vcat(([v myparse(v) e myparse(v)==e] for (v,e) in zip(strings_to_test,expected_output))...)
12×4 Matrix{Any}:
 "1°N"   (1.0, 0.0, 0.0)     (1.0, 0.0, 0.0)     true
 "1°S"   (-1.0, -0.0, -0.0)  (-1.0, -0.0, -0.0)  true
 "1°E"   (1.0, 0.0, 0.0)     (1.0, 0.0, 0.0)     true
 "1°W"   (-1.0, -0.0, -0.0)  (-1.0, -0.0, -0.0)  true
 "1'N"   (0.0, 1.0, 0.0)     (0.0, 1.0, 0.0)     true
 "1'S"   (-0.0, -1.0, -0.0)  (-0.0, -1.0, -0.0)  true
 "1'E"   (0.0, 1.0, 0.0)     (0.0, 1.0, 0.0)     true
 "1'W"   (-0.0, -1.0, -0.0)  (-0.0, -1.0, -0.0)  true
 "1\"N"  (0.0, 0.0, 1.0)     (0.0, 0.0, 1.0)     true
 "1\"S"  (-0.0, -0.0, -1.0)  (-0.0, -0.0, -1.0)  true
 "1\"E"  (0.0, 0.0, 1.0)     (0.0, 0.0, 1.0)     true
 "1\"W"  (-0.0, -0.0, -1.0)  (-0.0, -0.0, -1.0)  true

Finally, there are plenty of corner cases that this PR still gets wrong I'm sure. I just thought it might be useful to publish it here for more knowledgable eyes to look at it! 🙂

@briochemc briochemc marked this pull request as draft June 7, 2021 14:44
@mileslucas
Copy link
Member

So it seems like astropy uses two different conventions, for their standard tuples the sign is propagated through all the constituents and for the signed tuples they add a fourth field with the sign.

What is the benefit to having the sign in each token? How does that affect user inputs? e.g., currently the following three are equivalent in sort of a "most significant sign" arrangement in the function that combines the constituents, so there's no need to carry the sign in all the tokens.

dms2rad((-1, 2, 3))
dms2rad((-1, -2, -3))
dms2rad(-1, -2, 3))

@briochemc
Copy link
Author

I think — but I'm likely wrong too — that users that input an angle with a sign (+ or -) or a cardinal direction (N, E, S, or W) always place the sign in front of the first digit or the direction after the last digit and expect it to have low precedence. I.e., -1:2:3, -1°2'3", and 1°2'3"W, are meant to be equivalent to -(1:2:3) or (1:2:3)W, which should be evaluated as -(1 + 2/60 + 3/60). This is what this PR tries to address.

FWIW, I don't think there are users out there that input things like 1:-2:3 where the sign is alternating between. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Accept more formats
2 participants