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

Implement ability to hide warnings in VO table parsing #8715

Merged
merged 25 commits into from May 23, 2019

Conversation

Projects
None yet
4 participants
@astrofrog
Copy link
Member

commented May 16, 2019

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

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

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

@astrofrog astrofrog marked this pull request as ready for review May 16, 2019

@astrofrog astrofrog requested review from pllim, taldcroft, eteq, bsipocz and keflavich May 16, 2019

@bsipocz
Copy link
Member

left a comment

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()

Show resolved Hide resolved CHANGES.rst
Show resolved Hide resolved astropy/visualization/wcsaxes/core.py Outdated
@pllim

This comment has been minimized.

Copy link
Member

commented May 17, 2019

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.

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

@codecov

This comment has been minimized.

Copy link

commented May 17, 2019

Codecov Report

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

Impacted file tree graph

@@            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

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

@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
Copy link
Member

left a comment

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... 😹

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

This comment has been minimized.

Copy link
@pllim

pllim May 17, 2019

Member

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:

This comment has been minimized.

Copy link
@astrofrog

astrofrog May 17, 2019

Author Member

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).

Show resolved Hide resolved astropy/io/votable/exceptions.py Outdated
Show resolved Hide resolved astropy/io/votable/exceptions.py Outdated
Show resolved Hide resolved astropy/io/votable/exceptions.py Outdated
Show resolved Hide resolved astropy/io/votable/table.py Outdated
Show resolved Hide resolved astropy/io/votable/table.py Outdated
Show resolved Hide resolved astropy/io/votable/table.py Outdated
Show resolved Hide resolved astropy/io/votable/table.py
Show resolved Hide resolved astropy/io/votable/table.py
Show resolved Hide resolved astropy/io/votable/tree.py Outdated
@bsipocz

This comment has been minimized.

Copy link
Member

commented May 17, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

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

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

@pllim
Copy link
Member

left a comment

Thanks for the extensive tests! This is getting close.

Show resolved Hide resolved astropy/io/votable/__init__.py Outdated
Show resolved Hide resolved astropy/io/votable/connect.py Outdated
Show resolved Hide resolved astropy/io/votable/converters.py Outdated
Show resolved Hide resolved astropy/io/votable/exceptions.py Outdated
Show resolved Hide resolved astropy/io/votable/exceptions.py Outdated
Show resolved Hide resolved astropy/io/votable/table.py
Show resolved Hide resolved astropy/io/votable/table.py
Show resolved Hide resolved astropy/io/votable/tests/table_test.py Outdated
Show resolved Hide resolved astropy/io/votable/tests/table_test.py Outdated
Show resolved Hide resolved astropy/io/votable/tests/table_test.py Outdated

review has been addressed

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

@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
Copy link
Member

left a comment

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

Show resolved Hide resolved astropy/io/votable/tests/converter_test.py Outdated
Show resolved Hide resolved astropy/io/votable/tests/table_test.py Outdated
Show resolved Hide resolved astropy/io/votable/tests/table_test.py Outdated
Show resolved Hide resolved astropy/io/votable/tests/table_test.py Outdated
Show resolved Hide resolved astropy/io/votable/tests/table_test.py Outdated
Show resolved Hide resolved astropy/io/votable/tests/vo_test.py Outdated
Show resolved Hide resolved astropy/io/votable/tests/vo_test.py Outdated
Show resolved Hide resolved astropy/io/votable/tree.py Outdated
Show resolved Hide resolved astropy/io/votable/tree.py Outdated
Show resolved Hide resolved astropy/io/votable/tree.py Outdated
@pllim

pllim approved these changes May 22, 2019

Copy link
Member

left a comment

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

@pllim

This comment has been minimized.

Copy link
Member

commented May 22, 2019

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!

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

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

(rebased to get rid of the docs build error)

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

15 checks passed

astropy-bot:changelog Changelog entry consistent with milestone
astropy-bot:milestone This pull request has a milestone set.
ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl202 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl212 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl222 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl300 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpldev Your tests passed on CircleCI!
Details
codecov/patch 94.44% of diff hit (target 86.95%)
Details
codecov/project 86.97% (+0.01%) compared to b2b0717
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
giles Click details to preview the documentation build
Details
@pllim

This comment has been minimized.

Copy link
Member

commented May 23, 2019

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.