Refactor DS9 parser

[joleroi]

This builds on #128 and #129

Let's iterate on those first and then discuss this.

This PR:

add the line and circular annulus regions to the DS9 parsers. This should improve the coverage to a satisfying level. The parser will be restructured in a follow-up PR.

[keflavich]

-1 n

[joleroi]

Alright, with this I guess we can read all the regions we want to support for now. The coverage in https://github.com/joleroi/regions/blob/ds9_parser/dev/regions_pyregion_comparison.csv is still not super-high, but that's because the files are full of pandas, epandas, and bpandas.

We should, however, raise a Warning if e.g. an annulus or an ellipse with several radii is read, I will add this to this PR.

[joleroi]

@cdeil Note that the incapability to read pandas etc. is uncorrelated with the implementation of the parser. If someone feels like implementing these regions in this package, it's straightforward to add them to our parser.

[joleroi]

Update: I refactored the DS9 parser to (hopefully) make it more readable. See the docstrings for a description.

[joleroi]

Ready for review

[cdeil]

I left two minor inline comments.

How good are the warning and error messages when parsing errors or warnings occur?
Do you give a line number?
(recently I've been using yamllint, and there with the line numbers it's a pleasure to find the issues in the input files: https://github.com/adrienverge/yamllint#screenshot).

[cdeil]

Can we use OrderedDict everywhere instead of dict and preserve order?
In my experience this helps with readability for users, and also testing / debugging for developers.
We can't achieve round-tripping, but order helps a bit, no?

[cdeil]

Add blank linkes to make it clearer which comment is for which variable?

[astrofrog]

@joleroi - note that now that #132 is merged, the definition of width and height for the ellipse refer to the full size (not half-size) so can you make sure the ds9 reader/writer is consistent with that? (note that the conflict in write_ds9 is due to this change)

[cdeil]

@joleroi - Can you please rebase this PR and get this in?
I've added this to the 0.3 milestone.

[joleroi]

can you make sure the ds9 reader/writer is consistent with that?

@astrofrog This is the expected behaviour isn't it?

In [15]: ds9="galactic\nellipse(42,43, 1, 3, 45) # color=green"

In [16]: from regions import DS9Parser

In [17]: parser = DS9Parser(ds9)

In [18]: parser.run()

In [19]: print(parser.shapes[0].to_region())
Region: EllipseSkyRegion
center: <Galactic Coordinate: (l, b) in deg
    [( 42.,  43.)]>
width: 1.0 deg
height: 3.0 deg
angle: 45.0 deg

This PR is ready to be merged. There should be a follow-up PR, to update the DS9 Writer to also go via regions.Shape (in case we want to stick with this API).

[astrofrog]

@joleroi - yes that behavior looks correct, thanks!

[astrofrog]

@joleroi - there is a minor failure due to PEP8 stuff: https://travis-ci.org/astropy/regions/jobs/233214729

Otherwise I'm happy with this refactoring. In future once we implement other serializations, we might want to see if we can refactor to share some code between different serializations, but for now this looks good to me.

[keflavich]

Now that, again, it's too late, I tried to use this and immediately encountered a failure:

regstr = 'galactic; circle(+0:14:26.064,+0:00:45.206,30.400")'
from regions.io import ds9
dd = ds9.DS9Parser(regstr)
dd.run()

result:

  File "/Users/adam/repos/astropy-regions/regions/io/ds9/read.py", line 97, in <listcomp>
    ang = tuple([float(x) for x in string_rep.split(":")])
ValueError: could not convert string to float: 'e+0'

This case worked before, so we need to fix it and add a regression test.

[keflavich]

Also, now that I understand what's going on a bit better, I'm not a fan of the __init__/run model; is there any need for this to be class-based rather than functional?

[joleroi]

This case worked before, so we need to fix it and add a regression test.

I'll have a look

is there any need for this to be class-based rather than functional?

I don't think there is a killer argument for one of both approaches, so if others also think it should be changed back to a functional approach I am happy with it.

UPDATE: In any case we could add a function that runs the class based API behind the scenes

[cdeil]

No opinion on functional vs class-based code organisation from me.

I do think the intermediate representation as Shape / ShapeList is nice. It could make sense to use it in the writer also, no?

I did open up the current files in PyCharm and it points out a few issues:

• DS9RegionParser docstring mentions include parameter which doesn't exit.
• DS9Parser.parse_region takes include as parameter but doesn't use it
• Shape.to_region raises DS9RegionParserError which isn't imported

I'm not opening a PR because then we would probably start editing the same lines. Let me know if you want me to open a PR or if one of you can address these.

[joleroi]

DS9Parser.parse_region takes include as parameter but doesn't use it
Shape.to_region raises DS9RegionParserError which isn't imported

fixed in #135

[joleroi]

I do think the intermediate representation as Shape / ShapeList is nice. It could make sense to use it in the writer also, no?

👍

[keflavich]

I'm fine with having functions that wrap the class-based approach. My issue is probably just one of aesthetics; I don't like things that work like:

blah = new_object()
blah.exec()

because it's changing state; I feel the state change should be run at __init__ time. Is there any reason __init__ should not call .run in the parser (at least by default)?

[joleroi]

Is there any reason init should not call .run in the parser (at least by default)?

I (personally, with my limited experience) don't like it if I initialize a class and immediately get an error (e.g. if parsing failed), so I usually follow this 'init - run' pattern. That said, if other also want to do stuff on __init__ I'm happy with it.

[keflavich]

I lean the other way; I would much prefer to have the class fail on initialization if it's going to fail; what good is a Parser class that contains only failed data? However, I'm not sure there's any way to argue this either direction except based on personal preference.

[cdeil]

I just had a look at the new parsing code for the first time ... here's some feedback:

• Agree with keflavich that having to type 3 lines (create parser, run it, access result) to parse is cumbersome. So +1 to add a function to do that. Whether this function does the 3 lines in the background or you do a larger code re-organisation, I don't care.
• At http://astropy-regions.readthedocs.io/en/latest/ds9.html it mentions ShapeList and Shape, but the links to the API docs are broken. Expose as part of the public API?
• There's a DS9Parser and DS9RegionParser class with the same 1-line summary docstring. This is super confusing. Do we need both?
• I tried the example from https://pyregion.readthedocs.io/en/latest/getting_started.html and added a formatting error to see if your parser gives a good error message (mentioning the line number. It doesn't fail at all, I think this is a bug? And your objects like Shapelist and Shape don't have reprs, which is annoying for debugging.

In [10]: region_string = """
    ...: # Region file format: DS9 version 4.1
    ...: # Filename: test01.fits
    ...: global color=green dashlist=8 3 width=1 font="helvetica 10 normal" select=1 highlite=1 dash=0 fixed=0 edit=1 move=1 delete=1 include=1 source=1
    ...: fk5
    ...: circle(11:24:24.230,-59:15:02.20,18.5108") # color=cyan background
    ...: box(11:24:39.213,-59:16:53.91,42.804",23.616",19.0384,kronka) # width=4
    ...: """

In [11]: from regions.io.ds9 import DS9Parser

In [12]: DS9Parser(region_string)
Out[12]: <regions.io.ds9.read.DS9Parser at 0x10b410588>
In [15]: parser = DS9Parser(region_string)

In [16]: parser.run()

In [17]: parser.shapes
Out[17]: 
[<regions.io.ds9.core.Shape at 0x10b0b38d0>,
 <regions.io.ds9.core.Shape at 0x10b0b3ac8>]

In [18]: print(parser.shapes)
[<regions.io.ds9.core.Shape object at 0x10b0b38d0>, <regions.io.ds9.core.Shape object at 0x10b0b3ac8>]

In [19]: print(parser.shapes[0])
Shape
Coord sys : fk5
Region type : circle
Coord: [<Angle 11.406730555555557 hourangle>, <Angle -59.25061111111111 deg>, <Quantity 18.5108 arcsec>]
Meta: OrderedDict([('color', 'cyan background'), ('dashlist', '8 3 '), ('width', '1'), ('font', '"helvetica 10 normal" '), ('select', '1'), ('highlite', '1'), ('dash', '0'), ('fixed', '0'), ('edit', '1'), ('move', '1'), ('delete', '1'), ('include', '1'), ('source', '1')])
Composite: False
Include: True

I don't have time to help with regions development this week.
Probably given this state we shouldn't do an 0.3 release, no?

[joleroi]

I'll do a follow up PR tomorrow or next week.