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

Mast doc cleanup #1975

Merged
merged 8 commits into from
Mar 1, 2022
Merged

Mast doc cleanup #1975

merged 8 commits into from
Mar 1, 2022

Conversation

tinuademargaret
Copy link
Contributor

No description provided.

@astropy-bot astropy-bot bot added the mast label Feb 2, 2021
@tinuademargaret tinuademargaret marked this pull request as draft February 2, 2021 17:19
@tinuademargaret tinuademargaret added this to the v0.4.2 milestone Feb 2, 2021
@bsipocz bsipocz modified the milestones: v0.4.2, v0.4.3 May 15, 2021
@bsipocz bsipocz removed this from the v0.4.3 milestone Jul 7, 2021
@ceb8 ceb8 self-requested a review February 13, 2022 23:12
@ceb8 ceb8 added this to the v0.4.6 milestone Feb 21, 2022
@ceb8 ceb8 marked this pull request as ready for review February 21, 2022 22:20
@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #1975 (376b0bb) into main (f56d13a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1975   +/-   ##
=======================================
  Coverage   63.08%   63.08%           
=======================================
  Files         131      131           
  Lines       17005    17005           
=======================================
  Hits        10728    10728           
  Misses       6277     6277           

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 f56d13a...376b0bb. Read the comment docs.

@ceb8
Copy link
Member

ceb8 commented Feb 21, 2022

@bsipocz I rebased, and then also went through and fixed the code block indentation issue.

Now it's passing all the tests, but there is a weird extra code block bar at the top of each example (https://astroquery--1975.org.readthedocs.build/en/1975/mast/mast.html#module-astroquery.mast) has this come up with any of Tinuade's other PRs like this? I don't know how to fix it because all the syntax looks right. Ideas?

@bsipocz
Copy link
Member

bsipocz commented Feb 22, 2022

I don't know how to fix it because all the syntax looks right. Ideas?

@ceb8 - yes, you need to remove all the .. code-block:: python lines from the docs, that should fix this empty cell problem.

@ceb8
Copy link
Member

ceb8 commented Feb 22, 2022

@bsipocz Thank you!!!!

@bsipocz
Copy link
Member

bsipocz commented Feb 22, 2022

Now it's passing all the tests

You mean remotes, too? I still see failures with the rst file.

@ceb8
Copy link
Member

ceb8 commented Feb 22, 2022

You mean remotes, too? I still see failures with the rst file.

Working on the remotes right now. Stay tuned.

@ceb8
Copy link
Member

ceb8 commented Feb 23, 2022

@bsipocz Should be good to go now.

@@ -142,7 +142,7 @@ To list data missions archived by MAST and avaiable through `astroquery.mast`, u

>>> from astroquery.mast import Observations
...
>>> print(Observations.list_missions())
>>> print(Observations.list_missions()) # doctest: +IGNORE_OUTPUT
Copy link
Member

Choose a reason for hiding this comment

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

This may be valuable to keep, as it shouldn't really change that often.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I was unsure about ones like this because the point of the function is to decouple the list of missions from the codebase so that when new missions are added the astroquery code doesn't have to be updated every time. But also, you are correct, this doesn't change that often.

@@ -266,33 +266,31 @@ The below example illustrates downloading all product files with the extension "

>>> from astroquery.mast import Observations
...
>>> Observations.download_products('2003839997',
... productSubGroupDescription=["RAW", "UNCAL"],
>>> Observations.download_products('25119363',
Copy link
Member

Choose a reason for hiding this comment

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

Is this expected to change over time? Basically, I would keep everything output tested that is not expected to change as new data is added to the archive from ongoing missions.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think it is likely to change, the archive's linking between products is getting more sophisticated, with the result that more products will likely be returned from the query by obsid. Also the obsids are not guaranteed to stay consistent over time (which is maybe an argument for testing the output anyway so we know when to update the example).

@@ -494,11 +492,11 @@ The returned fields vary by catalog, find the field documentation for specific c
Some catalogs have a maximum number of results they will return.
If a query results in this maximum number of results a warning will be displayed to alert the user that they might be getting a subset of the true result set.

.. doctest-remote-data::
.. doctest-skip::
Copy link
Member

Choose a reason for hiding this comment

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

why switch to skip?

Copy link
Member

Choose a reason for hiding this comment

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

Because even though the output includes the max results warning the testing infrastructure considers it an unexpected exception. And obviously I don't want to change the example because the point is to show the user the max results warning.

Copy link
Member

Choose a reason for hiding this comment

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

pytest-doctestplus provides a directive for these kinds of situations, see https://github.com/astropy/pytest-doctestplus#showing-warnings

@bsipocz
Copy link
Member

bsipocz commented Feb 24, 2022

I'm afraid this now has to be rebased as #2095 has been merged.

@bsipocz bsipocz requested a review from ceb8 February 25, 2022 07:20
@bsipocz
Copy link
Member

bsipocz commented Mar 1, 2022

I went ahead and fixed the remaining examples, some were the new mission ones that needed the remote-data directive. Also, some more examples needed fixes, and I also did some minor cleanups. If tests pass, this is ready to go.

@bsipocz
Copy link
Member

bsipocz commented Mar 1, 2022

Also, I think the mission examples should not be ignored, but we cannot yet test them in the docs until the bug in #2313 is not fixed.

@bsipocz bsipocz merged commit 52cef2c into astropy:main Mar 1, 2022
@bsipocz
Copy link
Member

bsipocz commented Mar 1, 2022

Thanks @tinumide and @ceb8!

@bsipocz bsipocz mentioned this pull request Mar 12, 2022
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