ds9 reader corrections/improvements #90

keflavich · 2016-11-30T22:00:26Z

Mostly minor:

warn if a region is skipped when reading
read 'box' regions (we accepted only 'rectangle' before)
allow for whitespace after commas

is what ds9 actually uses for rectangles), adding a warning if the region isn't recognized, and allowing for whitespace (which ds9 can handle)

cdeil · 2016-11-30T22:05:34Z

@keflavich - Thanks!

Long-term I think an option to "ignore", "warn" or "error" on this would be best, like what the Python unicode encode / decode functions do.

For now: can you please add one test for one case where the warning is raised, asserting that it works?

keflavich · 2016-11-30T22:29:03Z

maybe kwarg should be skipped={'warn','ignore','error'}? What keyword do you prefer?

cdeil · 2016-12-01T14:45:23Z

maybe kwarg should be skipped={'warn','ignore','error'}? What keyword do you prefer?

I like errors for the name of the parse function argument, meaning "how to deal with parsing errors"

That's what Python uses for str.encode and bytes.decode:

In [6]: str.encode?
Docstring:
S.encode(encoding='utf-8', errors='strict') -> bytes

Encode S using the codec registered for encoding. Default encoding
is 'utf-8'. errors may be given to set a different error
handling scheme. Default is 'strict' meaning that encoding errors raise
a UnicodeEncodeError. Other possible values are 'ignore', 'replace' and
'xmlcharrefreplace' as well as any other name registered with
codecs.register_error that can handle UnicodeEncodeErrors.
Type:      method_descriptor

and

In [8]: bytes.decode?
Signature: bytes.decode(self, /, encoding='utf-8', errors='strict')
Docstring:
Decode the bytes using the codec registered for encoding.

encoding
  The encoding with which to decode the bytes.
errors
  The error handling scheme to use for the handling of decoding errors.
  The default is 'strict' meaning that decoding errors raise a
  UnicodeDecodeError. Other possible values are 'ignore' and 'replace'
  as well as any other name registered with codecs.register_error that
  can handle UnicodeDecodeErrors.
Type:      method_descriptor

cdeil

Is a AstropyUserWarning appropriate for parsing errors? Shouldn't it be a more specific DS9RegionParsingError to make it more obvious and easier to handle in other libraries?

cdeil · 2016-12-01T14:50:00Z

regions/io/read_ds9.py

@@ -192,20 +194,33 @@ def ds9_region_list_to_objects(region_list):
            else:
                raise ValueError("No central coordinate")
        else:
+            # Note: this should effectively never happen, because it would


I think this will happen all the time. People have invalid ds9 files and region_type. They mistype or whatever.

Can you change this code comment from "this should effectively never happen, ..." to something like "For invalid inputs, we do ..."

Is it possible to give the user a line number where the problem in their file occurs?
It will be really common and users will appreciate a helpful message.

I think this error actually can't occur by user error. If a user mistypes / enters an incorrect region type, the previous function, ds9_string_to_region_list, should fail. Only regions that are parsed by ds9_string_to_region_list but do not have a corresponding entry within ds9_region_list_to_objects should reach this point, and that should only happen if someone updating this code forgets to add a new entry in this function.

cdeil · 2016-12-01T14:51:00Z

regions/io/read_ds9.py

    """Parse ds9 region string to region objects

    Parameters
    ----------
    region_string : str
        DS9 region string
+    warn_skipped : bool
+        Print a warning if there is a skipped (commented) line?
+        Can set to ``False`` or ``'raise'`` if you want an exception instead.


Like I said in the other comment, I'd prefer to call it "error".
And as-is, the option is documented as bool, but you also accept a string raise.
That's no good.
Let's make it a multi-choice string option.

cdeil · 2016-12-01T14:53:04Z

regions/io/read_ds9.py

@@ -357,6 +385,19 @@ def line_parser(line, coordsys=None):
                language_spec[region_type] = itertools.chain((coordinate, coordinate), itertools.cycle((radius,)))

            return region_type, parsed_return, parsed_meta, composite, include
+    else:
+        # it is not clear when it is OK to warn


Can we resolve / improve the discussion on when to warn in this PR, and then change the comment?
It currently reads as an open issue "it is not clear ..."

Maybe just put whatever behaviour you like best, and establish it via tests?

keflavich · 2016-12-05T01:14:54Z

fyi, I'm pushing failing builds because I can't run tests locally; they all raise:
E OSError: Could not find lib c or load any of its variants [].

cdeil · 2016-12-05T07:13:06Z

E OSError: Could not find lib c or load any of its variants [].

If you can't figure it out, maybe try git clean -fdx and python setup.py test in master and paste the full output in a gist, and I'll try to help by having a look at the log.

keflavich · 2016-12-05T18:29:12Z

I can't figure it out, the problem persists, but I can run the tests directly now with

py.test regions/io/tests/test_ds9_language.py

keflavich · 2016-12-08T23:10:19Z

@cdeil I think this is ready for final review now

cdeil · 2016-12-09T08:16:55Z

@astrofrog or @joleroi - Do you have time for a quick review here?

cdeil · 2016-12-09T08:18:56Z

I think it's good that you introduce a specific DS9RegionParserWarning.
But one motivation for custom warning / error is that people can catch it, right?
So it should be added to __all__ and show up in the docs and be part of our public API, no?

For equivalent errors, should we stick with ValueError as you do here, or introduce DS9RegionParserError, to be equivalent with the warning?

cdeil · 2016-12-09T08:21:38Z

regions/io/tests/test_ds9_language.py

 from ..write_ds9 import write_ds9, ds9_objects_to_string

-_ASTROPY_MINVERSION = v.LooseVersion('1.1')
-_ASTROPY_VERSION = v.LooseVersion(astrov.version)
+_ASTROPY_MINVERSION = vers.LooseVersion('1.1')


We are working towards putting this as astropy.regions in the core package.
So there's no need to support old Astropy versions.

At the moment regions has to work with Astropy 1.2, and we can switch to 1.3 as soon as that is out.

So remove this xfail for Astropy < 1.1 here.

I don't see any reason to remove the xfail now, as long as regions is still an independent package. Sure, when we merge it into astropy-core, then it makes sense... but that's not now.

keflavich · 2016-12-09T17:58:10Z

I like the idea of adding a DS9RegionParserError as a subclass of ValueError.... I'll do it if I have time.

cdeil · 2016-12-10T12:10:31Z

I'm merging this now.

Thanks, @keflavich !

keflavich added 2 commits November 30, 2016 14:53

slightly improve the ds9 reader by adding the 'box' region type (which

2e96930

is what ds9 actually uses for rectangles), adding a warning if the region isn't recognized, and allowing for whitespace (which ds9 can handle)

change the log.warn to a real warning

1340757

cdeil added bug enhancement labels Nov 30, 2016

cdeil added this to the 0.2 milestone Nov 30, 2016

keflavich added 4 commits November 30, 2016 15:06

vizmeta -> visual and remove 'text' from viz_keywords (it is not one)

8d5956a

add a test for the warning and add some other warnings

25093b7

kwarg passing

4ada62d

clean up the test

541553c

don't warn about skipping blank lines...

4ba6a24

cdeil self-assigned this Dec 1, 2016

cdeil reviewed Dec 1, 2016

View reviewed changes

keflavich added 2 commits December 4, 2016 17:27

change error handling to @cdeil's recommendations

b105952

address some more of @cdeil's suggestions

57af932

another important enhancement (global handling) and got tests running

25fbd5b

unicode literal was breaking py2

21b32b3

cdeil reviewed Dec 9, 2016

View reviewed changes

keflavich added 2 commits December 9, 2016 10:58

add ds9regionparserwarning to __all__

ad18912

DS9 Region Parser Error added

bb8de37

cdeil merged commit 81d6439 into astropy:master Dec 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ds9 reader corrections/improvements #90

ds9 reader corrections/improvements #90

keflavich commented Nov 30, 2016

cdeil commented Nov 30, 2016

keflavich commented Nov 30, 2016

cdeil commented Dec 1, 2016

cdeil left a comment

cdeil Dec 1, 2016

keflavich Dec 1, 2016

cdeil Dec 1, 2016

cdeil Dec 1, 2016

keflavich commented Dec 5, 2016

cdeil commented Dec 5, 2016

keflavich commented Dec 5, 2016

keflavich commented Dec 8, 2016

cdeil commented Dec 9, 2016

cdeil commented Dec 9, 2016

cdeil Dec 9, 2016

keflavich Dec 9, 2016

keflavich commented Dec 9, 2016

cdeil commented Dec 10, 2016

ds9 reader corrections/improvements #90

ds9 reader corrections/improvements #90

Conversation

keflavich commented Nov 30, 2016

cdeil commented Nov 30, 2016

keflavich commented Nov 30, 2016

cdeil commented Dec 1, 2016

cdeil left a comment

Choose a reason for hiding this comment

cdeil Dec 1, 2016

Choose a reason for hiding this comment

keflavich Dec 1, 2016

Choose a reason for hiding this comment

cdeil Dec 1, 2016

Choose a reason for hiding this comment

cdeil Dec 1, 2016

Choose a reason for hiding this comment

keflavich commented Dec 5, 2016

cdeil commented Dec 5, 2016

keflavich commented Dec 5, 2016

keflavich commented Dec 8, 2016

cdeil commented Dec 9, 2016

cdeil commented Dec 9, 2016

cdeil Dec 9, 2016

Choose a reason for hiding this comment

keflavich Dec 9, 2016

Choose a reason for hiding this comment

keflavich commented Dec 9, 2016

cdeil commented Dec 10, 2016