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

Feature Request: Accept more formats #2

Closed
3 of 4 tasks
briochemc opened this issue Apr 21, 2021 · 9 comments · May be fixed by #4
Closed
3 of 4 tasks

Feature Request: Accept more formats #2

briochemc opened this issue Apr 21, 2021 · 9 comments · May be fixed by #4

Comments

@briochemc
Copy link

briochemc commented Apr 21, 2021

I'd like to suggest expanding the accepted coordinate formats. The goal would be able to parse these:

  • 1°1′1 (minute symbol made with \prime + TAB)
  • 1°1' (no seconds)
  • (no minutes)
  • 1°1'1W (North/South/West/East direction at the end)

Here is a little snippet to test these:

julia> strings_to_test = Dict(
           "expected"      => "1°1'1",
           "\\prime"       => "1°1′1",
           "no seconds"    => "1°1'",
           "no minutes"    => "",
           "NSEWdirection" => "1°1'1W"
       ) ;

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

julia> vcat(([k v myparse(v)] for (k,v) in strings_to_test)...)
5×3 Matrix{Any}:
 "expected"       "1°1'1"   (1.0, 1.0, 1.0)
 "no minutes"     ""      "does not parse"
 "NSEWdirection"  "1°1′1W"  "does not parse"
 "no seconds"     "1°1'"    "does not parse"
 "\\prime"        "1°1'1"   "does not parse"
@mileslucas
Copy link
Member

For partial angles like "1°1'"- are there real use-cases where coordinates are saved in a partial format? For REPL usage, the extra few characters to write ``"1°1'0"` seems like a much simpler solution than building in another two layers of logical complexity to the regex strings.

As for the other formats, I'm not too eager to build in every corner-case under the sun, but I see astropy supports a few of them so I guess feature parity wins out there.

@mileslucas
Copy link
Member

I see astropy supports the partial angles, too, so I'll add them into the PR I'm working on.

@briochemc
Copy link
Author

Yes, here is one example: https://www.sciencedirect.com/science/article/pii/S0009254119300075?via%3Dihub#t0015 in the wild. (I "manually" did the conversion from X°Y'W to -X-Y/60 in this case.)

I strongly agree about the REPL use case. But we shouldn't ignore the non-REPL use cases, right?

I think the more formats are supported, the better, but I don't want to sound like an entitled user! Feel free to chose and select what you think is worth implementing / including in the package!

@mileslucas
Copy link
Member

All the above are now supported after c6ec692

I just started a 0.1.1 release.

Regex is fun (:

@briochemc
Copy link
Author

briochemc commented Apr 23, 2021

This looks great! However, I think there is a bug or two remaining. I noticed that North/South is treated differently from East/West (one is for degrees, the other for hours). Could you explain why this is the case?

I did not tick the 4th example in the list at the start of this thread, namely 1°1'1W, because AstroAngles (v0.1.2) currently returns a positive value instead of a negative one.

Here is another MWE that could serve as a testbed (displayed are: original string, parsed output, expected output, and a boolean to test equality between the current and expected output):

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)  false
 "1'N"   (1.0, 0.0, 0.0)   (0.0, 1.0, 0.0)     false
 "1'S"   (-1.0, 0.0, 0.0)  (-0.0, -1.0, -0.0)  false
 "1'E"   (1.0, 0.0, 0.0)   (0.0, 1.0, 0.0)     false
 "1'W"   (1.0, 0.0, 0.0)   (-0.0, -1.0, -0.0)  false
 "1''N"  (1.0, 0.0, 0.0)   (0.0, 0.0, 1.0)     false
 "1''S"  (1.0, 0.0, 0.0)   (-0.0, -0.0, -1.0)  false
 "1''E"  (1.0, 0.0, 0.0)   (0.0, 0.0, 1.0)     false
 "1''W"  (1.0, 0.0, 0.0)   (-0.0, -0.0, -1.0)  false

As you can see the West direction is not taken into account (it's only implemented for hms) and in the absence of degrees the parsing silently assumes that minutes and seconds are degrees...

I'm also happy to try and submit a PR if the expected output above is what you'd want.

@mileslucas
Copy link
Member

I see the problem- I just don't understand it. Do you have a reference or can write out exactly how the directions should change the angle? I started with N/S and E/W because that's what made sense for e.g. ICRS coordinates. Similarly, the sign convention for hms is based off the hour angle parsing that astropy does- so again if you have some reference that would be the best clarification.

@briochemc
Copy link
Author

I just think that "1°W" should be parsed like "-1°E" (i.e., as a negative).

Although I'm not familiar with that package, looking at astropy's tests seems to confirm this (but maybe I'm confused?):

    a = Angle("3dN")
    assert str(a) == "3d00m00s"
    assert a.unit == u.degree
    a = Angle("3dS")
    assert str(a) == "-3d00m00s"
    assert a.unit == u.degree
    a = Angle("3dE")
    assert str(a) == "3d00m00s"
    assert a.unit == u.degree
    a = Angle("3dW")
    assert str(a) == "-3d00m00s"
    assert a.unit == u.degree
    a = Angle('10"')
    assert_allclose(a.value, 10.0)
    assert a.unit == u.arcsecond
    a = Angle("10'N")
    assert_allclose(a.value, 10.0)
    assert a.unit == u.arcminute
    a = Angle("10'S")
    assert_allclose(a.value, -10.0)
    assert a.unit == u.arcminute
    a = Angle("10'E")
    assert_allclose(a.value, 10.0)
    assert a.unit == u.arcminute
    a = Angle("10'W")
    assert_allclose(a.value, -10.0)
    assert a.unit == u.arcminute
    a = Angle('45°55′12″N')
    assert str(a) == '45d55m12s'
    assert_allclose(a.value, 45.92)
    assert a.unit == u.deg
    a = Angle('45°55′12″S')
    assert str(a) == '-45d55m12s'
    assert_allclose(a.value, -45.92)
    assert a.unit == u.deg
    a = Angle('45°55′12″E')
    assert str(a) == '45d55m12s'
    assert_allclose(a.value, 45.92)
    assert a.unit == u.deg
    a = Angle('45°55′12″W')
    assert str(a) == '-45d55m12s'
    assert_allclose(a.value, -45.92)
    assert a.unit == u.deg

@mileslucas
Copy link
Member

Okay, I've added the cardinal directions for both hms and dms. The convention will be "W" and "S" are negative.

I took a stab at implementing when only one digit is provided but isn't degrees, I didn't find a great solution.

@briochemc
Copy link
Author

briochemc commented Jun 7, 2021

I started a branch myself by splitting the huge regex in smaller chunks and I think that I stumbled onto an error.

What should "-1:2:3" parse to? IMHO the minus sign should propagate all the way, but currently the sign only matters for the degrees/hours. But maybe I'm wrong?

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 a pull request may close this issue.

2 participants