Implement ability to hide warnings in VO table parsing by astrofrog · Pull Request #8715 · astropy/astropy

astrofrog · 2019-05-16T09:22:35Z

This PR makes it possible to hide warnings when parsing VO tables, by replacing the boolean pedantic flag with a verify flag that can be ignore, warn, or exception (or should it be error?)

In addition, as discussed in #8028, this makes ignore the default. I think in general we should move to a model where we don't give 25 warnings when reading in a table where these are fixed anyway and beyond the user's control (basically the Robustness Principle of being liberal in what you accept)

I've tagged various people I think might be interested. If we go ahead with this I think in another PR we should also change the default for FITS to not show fixable warnings on input.

EDIT: Fix #8028

bsipocz

Overall I think is the right approach. My comments below are minor ones.

Also I would think that adding a 'fix' option for the same kwarg would be very nice, that would also nicely mirror the behaviour/options of the fits .verify()

pllim · 2019-05-17T09:13:58Z

or should it be error?

Naw, it is also exception on io.fits (http://docs.astropy.org/en/stable/io/fits/usage/verification.html), so in the spirit of consistency, we can use that.

codecov · 2019-05-17T09:50:22Z

Codecov Report

Merging #8715 into master will increase coverage by 0.01%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master    #8715      +/-   ##
==========================================
+ Coverage   86.95%   86.97%   +0.01%     
==========================================
  Files         399      399              
  Lines       59355    59373      +18     
  Branches     1100     1100              
==========================================
+ Hits        51613    51637      +24     
+ Misses       7101     7095       -6     
  Partials      641      641

Impacted Files	Coverage Δ
astropy/io/votable/validator/result.py	`13.94% <0%> (ø)`	⬆️
astropy/io/votable/converters.py	`93.85% <100%> (ø)`	⬆️
astropy/io/votable/exceptions.py	`93.6% <100%> (+0.34%)`	⬆️
astropy/io/votable/tree.py	`85.98% <100%> (+0.28%)`	⬆️
astropy/io/votable/__init__.py	`100% <100%> (ø)`	⬆️
astropy/io/votable/connect.py	`77.41% <100%> (ø)`	⬆️
astropy/io/votable/table.py	`86.46% <94.11%> (+0.63%)`	⬆️
astropy/config/configuration.py	`83.78% <0%> (+0.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2b0717...94826f0. Read the comment docs.

astrofrog · 2019-05-17T09:53:36Z

@bsipocz:

Also I would think that adding a 'fix' option for the same kwarg would be very nice, that would also nicely mirror the behaviour/options of the fits .verify()

This would require a lot more work since at the moment there is no control over whether things are fixed or not. They are always fixed, and it could be that the VO table can't be interpreted if not fixed (a little different to FITS headers which can be sub-optimal but still work). I'd be tempted to keep just these three options for now, and then consider adding more in future if people really want?

pllim

My preliminary review from eyeballing the code. When this is more stable, I would like to take it for a spin before merge. Thanks! I think this will be one of the more popular PRs for 4.0... 😹

pllim · 2019-05-17T09:31:25Z

-        'When True, treat fixable violations of the VOTable spec as exceptions.',
-        aliases=['astropy.io.votable.table.pedantic'])
+    verify = _config.ConfigItem(
+        'ignore',


Now that I look at this, I am actually not sure if alias would work if the config type has changed. When cfgtype is not provided, as in the case here, it is inferred from the default value. So with this change, its type is now string, where it used to be boolean. That is going to choke the validator when someone has an older astropy.cfg with, say, pedantic = True set. A possible solution is to explicitly state the cfgtype as 'option' and provide all the 5 possible options (3 new ones and 2 boolean values).

References:

http://docs.astropy.org/en/latest/config/index.html

http://www.voidspace.org.uk/python/validate.html#the-standard-functions

For some reason it still doesn't work right if I specify option with mixed types, and it always gets converted to a string. But that's ok, we can still be backward compatible by just checking for False and True as strings when using conf.verify. I've added handling of this as well as an extensive test of setting the configuration items (both verify and pedantic).

bsipocz · 2019-05-17T11:33:13Z

@astrofrog

Also I would think that adding a 'fix' option for the same kwarg would be very nice, that would also nicely mirror the behaviour/options of the fits .verify()

out of band discussion showed me why this is not a good idea, so please ignore it, it shouldn't actually fix anything.

astrofrog · 2019-05-17T12:27:51Z

@bsipocz @pllim - I've implemented all your comments. Feel free to take this for a spin!

pllim

Thanks for the extensive tests! This is getting close.

review has been addressed

astrofrog · 2019-05-17T17:59:32Z

@pllim - I had to change some of the code in parse back to being able to handle booleans, because we need to catch the case where the user calls the function with pedantic=True for instance. Otherwise I think I've accepted/implemented all your suggestions!

pllim

Sorry, I keep finding stuff to comment on... 😬

pllim

I think 🐴 is 💀 . Thanks for your patience with my reviews!

pllim · 2019-05-22T19:29:39Z

p.s. I am excited to see how many warnings disappear in #7928 once this is merged and I rebase that one on top of this one!

Co-Authored-By: P. L. Lim <[email protected]>

…and pedantic options

Co-Authored-By: P. L. Lim <[email protected]>

…sing conf.verify

astrofrog · 2019-05-23T11:06:50Z

(rebased to get rid of the docs build error)

pllim · 2019-05-23T15:41:43Z

astrofrog added the io.votable label May 16, 2019

astrofrog added this to the v4.0 milestone May 16, 2019

astrofrog force-pushed the votable-pedantic branch 2 times, most recently from b5d26a7 to 3a3eb49 Compare May 16, 2019 10:33

astrofrog marked this pull request as ready for review May 16, 2019 19:02

astrofrog requested review from bsipocz, eteq, keflavich, pllim and taldcroft May 16, 2019 19:02

bsipocz previously requested changes May 17, 2019

View reviewed changes

Comment thread CHANGES.rst

Comment thread astropy/visualization/wcsaxes/core.py Outdated

keflavich approved these changes May 17, 2019

View reviewed changes

astrofrog force-pushed the votable-pedantic branch from 3a48b46 to 98decd9 Compare May 17, 2019 09:50

pllim requested changes May 17, 2019

View reviewed changes

astrofrog commented May 17, 2019

View reviewed changes

Comment thread astropy/io/votable/converters.py Outdated

astrofrog force-pushed the votable-pedantic branch 3 times, most recently from c197aa5 to 997307c Compare May 17, 2019 13:55

pllim requested changes May 17, 2019

View reviewed changes

pllim reviewed May 22, 2019

View reviewed changes

pllim approved these changes May 22, 2019

View reviewed changes

pllim added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label May 22, 2019

astrofrog and others added 18 commits May 23, 2019 12:05

Apply suggestions from code review

e94daba

Co-Authored-By: P. L. Lim <[email protected]>

Added more extensive tests of setting configuration items for verify …

82283cd

…and pedantic options

Fix parse() following removal of pedantic option

d790ae0

Updated package name in documentation for exceptions/warnings

86abb55

Remove pedantic from docstring

ee9701e

Added a .. versionchanged entry for the change from pedantic to verify

51335e6

Fix logic in if statement

ffd55f4

Remove unused code

13c67cc

Fix defaults for behavior in warning/exception-handling code

2904429

Fix default value of verify in converters

fb3c873

Apply suggestions from code review

61f9e98

Co-Authored-By: P. L. Lim <[email protected]>

Refactor tests into class and split up tests

12ecba0

Change logic back for verify as booleans

d26a816

Apply suggestions from code review

c9cf40e

Co-Authored-By: P. L. Lim <[email protected]>

Fixed another instance of warn -> ignore

d21679c

Co-Authored-By: P. L. Lim <[email protected]>

Make sure we don't actually emit a deprecation warning yet when acces…

ca8462a

…sing conf.verify

Don't specify verify='ignore' in tests since it is the default

c5add16

Use raw string

94826f0

astrofrog force-pushed the votable-pedantic branch from 125d8d0 to 94826f0 Compare May 23, 2019 11:06

pllim merged commit 2d99bed into astropy:master May 23, 2019

pllim mentioned this pull request Jun 20, 2019

Truly deprecate pedantic option in VO table #8887

Closed

pllim mentioned this pull request Aug 20, 2019

Remove default warning when replacing a column that is a slice #9144

Merged

pllim mentioned this pull request Sep 23, 2019

Allow custom root directory for config #8237

Merged

2 tasks done

eerovaher mentioned this pull request Apr 26, 2023

MNT: Remove pedantic option from astropy.io.votable #14669

Merged

Conversation

astrofrog commented May 16, 2019 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pllim commented May 17, 2019

Uh oh!

codecov Bot commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

astrofrog commented May 17, 2019

Uh oh!

pllim left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim May 17, 2019

Choose a reason for hiding this comment

Uh oh!

astrofrog May 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bsipocz commented May 17, 2019

Uh oh!

Uh oh!

astrofrog commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

astrofrog commented May 17, 2019

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented May 22, 2019

Uh oh!

astrofrog commented May 23, 2019

Uh oh!

pllim commented May 23, 2019

Uh oh!

Reviewers

astrofrog commented May 16, 2019 •

edited by pllim

Loading

codecov Bot commented May 17, 2019 •

edited

Loading

pllim left a comment •

edited

Loading

astrofrog commented May 17, 2019 •

edited

Loading