JuliaAstro/AstroAngles.jl

Feature Request: Accept more formats

briochemc opened this issue · 9 comments

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"

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.

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

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!

All the above are now supported after c6ec692

I just started a 0.1.1 release.

Regex is fun (:

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.

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.

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

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.

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?