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

DOC: Change link targets at the protocol overview into pyVO docs; add brief intro for each service protocol #372

Merged
merged 4 commits into from
Nov 2, 2022

Conversation

chmmao
Copy link
Contributor

@chmmao chmmao commented Oct 5, 2022

Background:
When I clicked on "TAP", for instance, on the opening page of the pyVO docs, I ended up at the IVOA documentation rather than the pyVO docs on how to use TAP, which is in my opinion not so convenient and straightforward.

Suggestion:
This PR attempts to improve the user experience(for a newcomer like me) by changing the link target of each service protocol to the position in pyVO docs on how to use them; it also adds brief introductions as to what the protocols are, and that's also where the former links into the IVOA doc repo now reside.

bsipocz
bsipocz previously requested changes Oct 5, 2022
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I leave the review of the actual text to others, but overall very much like the logic of the changes of keeping the docs self-contained and linking out to the difficult-to-read standard documents only as an external reference.

The minor change that I would like to see is to add internal sphinx references rather than use HTML anchors, I'm happy to help to figure those out.

docs/index.rst Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Merging #372 (8f7610d) into main (a7f247f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #372   +/-   ##
=======================================
  Coverage   78.55%   78.55%           
=======================================
  Files          47       47           
  Lines        5564     5564           
=======================================
  Hits         4371     4371           
  Misses       1193     1193           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bsipocz bsipocz added this to the v1.4.1 milestone Oct 6, 2022
@bsipocz bsipocz dismissed their stale review October 6, 2022 17:58

Review has been addressed

Copy link
Contributor

@andamian andamian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new paragraphs come straight from the corresponding specs. If we use them, we might need to give credit where is due by quoting and mentioning the source. For ex:
From TAP specification:
"<TAP paragraph>"

database tables. Access is provided for both database and table metadata
as well as for actual table data. This version of the protocol includes
support for multiple query languages, including queries specified using
the Astronomical Data Query Language within an integrated interface.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please introduce the ADQL acronym here by adding (ADQL). A link to the IVOA would also be nice.

queries against an arbitrarily large list of astronomical targets,
providing a simple spatial cross-matching capability.
More sophisticated distributed cross-matching capabilities are possible by
orchestrating a distributed query across multiple TAP services.

Unlike the other services, this one works with tables queryable by an sql-ish
language called *ADQL* instead of predefined search constraints.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this paragraph since it's just duplicating the info now.

The `Simple Image Access (SIA) <https://www.ivoa.net/documents/SIA/>`_ protocol
provides capabilities for the discovery, description, access, and retrieval
of multi-dimensional image datasets, including 2-D images as well as datacubes
of three or more dimensions. SIA data discovery is based on the ObsCore Data Model,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IVOA reference missing

which primarily describes data products by the physical axes (spatial, spectral,
time, and polarization). Image datasets with dimension greater than 2 are often
referred to as datacubes, cube or image cube datasets and may be considered examples
of hypercube or n-cube data. In this document the term "image" refers to general
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From "In this document ..." onwards, the info is not relevant to this documentation

SSA-defined data model at access time by the service, so that client analysis
programs do not have to be familiar with the idiosyncratic details of
each data collection to be accessed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a bit too long and detailed as compared with the other ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your review, I made a couple of changes to the texts according to your suggestions. Please have a look if they are appropriate now.

Copy link
Contributor

@andamian andamian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@andamian
Copy link
Contributor

andamian commented Nov 1, 2022

Anyone else wants to comment?

@msdemlei
Copy link
Contributor

msdemlei commented Nov 2, 2022 via email

@andamian andamian merged commit 58863a6 into astropy:main Nov 2, 2022
@andamian
Copy link
Contributor

andamian commented Nov 2, 2022

Merged. @ChuanmingMao - thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants