Gaia documentation cleanup #1973
Conversation
There was a problem hiding this comment.
This pull request was opened quite some time ago, and in the meanwhile the default verbosity of Gaia has been changed by #2085, so the addition of info messages to the example outputs should be undone.
Furthermore, there seem to be quite a few examples with the IGNORE_OUTPUT directive, but I couldn't tell why.
| .. doctest-remote-data:: | ||
| .. doctest-skip:: |
There was a problem hiding this comment.
Aren't these directives conflicting?
|
|
||
| >>> import astropy.units as u | ||
| >>> from astropy.coordinates import SkyCoord | ||
| >>> from astroquery.gaia import Gaia | ||
| >>> from astroquery.gaia import Gaia # doctest: +IGNORE_OUTPUT |
There was a problem hiding this comment.
It used to be the case that importing Gaia resulted in log messages that are not written out in this example, so the directive to ignore output would have been appropriate. However, the default verbosity setting of Gaia was changed in #2085, so this line should no longer produce any output and the directive is not needed.
| >>> | ||
| >>> coord = SkyCoord(ra=280, dec=-60, unit=(u.degree, u.degree), frame='icrs') | ||
| >>> width = u.Quantity(0.1, u.deg) | ||
| >>> height = u.Quantity(0.1, u.deg) | ||
| >>> r = Gaia.query_object_async(coordinate=coord, width=width, height=height) | ||
| INFO: Query finished. [astroquery.utils.tap.core] |
There was a problem hiding this comment.
These info messages should not appear now that #2085 has been merged.
Actually they still appear.
| 1635721458409799680 Gaia DR2 6632920344009005184 ... 0.9999925631357645 | ||
| 1635721458409799680 Gaia DR2 6636406929741393024 ... 0.9999942477166328 | ||
| 1635721458409799680 Gaia DR2 6636389951735167872 ... 0.9999964452249156 | ||
| Length = 113243 rows |
There was a problem hiding this comment.
This is a lot of rows. Wouldn't it be better to change the examples to avoid such queries?
| public.hipparcos | ||
| ... | ||
| gaiadr2.gaia_source | ||
| ... print(table.get_qualified_name()) # doctest: +IGNORE_OUTPUT |
There was a problem hiding this comment.
Wouldn't it be useful to test this output?
| >>> for column in (gaiadr2_table.columns): | ||
| >>> print(column.name) | ||
|
|
||
| ... print(column.name) # doctest: +IGNORE_OUTPUT |
| >>> r = job.get_results() | ||
| >>> print(r['solution_id']) | ||
|
|
||
| >>> print(r['solution_id']) # doctest: +IGNORE_OUTPUT |
|
|
||
| >>> print(job) | ||
|
|
||
| >>> print(job) # doctest: +IGNORE_OUTPUT |
There was a problem hiding this comment.
It looks like this is being ignored because the automatically generated output file name changes from run to run, but wouldn't it be better to provide a filename to the launch_job query above?
The main problem with this example is that it produces a file that persists after the tests have finished, so it is actually better to skip this test entirely.
| >>> r = job.get_results() | ||
| >>> print(r) | ||
|
|
||
| >>> print(r) # doctest: +IGNORE_OUTPUT |
|
I'm sorry @ceb8, but I have to revert your approval as this PR is still in a WIP stage and failing the remote tests, and having the green tickmark on it is misleading. |
bsipocz
dismissed
ceb8’s stale review
Remote tests are failing, rebase and fixes are required
|
@eerovaher - I'm not responding to all your review comments, but give the high level overview: This PR had the purpose to switch our documentation to be tested. You see multiple similar ones, some are merged some are still WIP and therefore you see a few inconsistencies, some of you have pointed out. If you have some energy, picking any of these up is more than welcome, I would only ask to keep Tinu's commits at the beginning of your PR to acknowledge her initial work on this topic. Overall logic: we doctest, but not religiously, e.g. if we expect an answer to change over time as e.g. more data is added to the remote service, then we don't test the output, just that it runs, so you can use |
The already published Gaia data releases are not modified, so we only expect the output to change when a new data release comes out and the In any case I have opened #2283 to finish what was started here. |
Sure, that's true for the case of Gaia, assuming no change has been done in the upstream server of how to present the data. Anyway, my point is more generic for these doc PRs that enable doctesting: we would love to enable testing and if the changing nature of exact output is the bottleneck, then we should just ignore what's exactly in the output, after all tricky cases should be covered in the actual code tests. |
|
Closing because superseded by #2283. |
No description provided.