Skip to content

Conversation

mstcyr2
Copy link
Contributor

@mstcyr2 mstcyr2 commented May 31, 2023

Removing redundancies and excess while preserving the flow of the document.

@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Merging #2743 (be4fa51) into main (e9d229f) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head be4fa51 differs from pull request most recent head b1ae163. Consider uploading reports for the commit b1ae163 to get more accurate results

@@            Coverage Diff             @@
##             main    #2743      +/-   ##
==========================================
- Coverage   66.10%   66.09%   -0.01%     
==========================================
  Files         235      235              
  Lines       18069    18071       +2     
==========================================
  Hits        11944    11944              
- Misses       6125     6127       +2     

see 1 file with indirect coverage changes

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

@bsipocz
Copy link
Member

bsipocz commented Jun 1, 2023

@mstcyr2 @jaymedina - At this point it may make sense to chop up the mast documentation into several pages, both into separate rst files that are lined from mast.rst. That would in fact help with testing the documentation, too (see #2505)

@jaymedina
Copy link
Contributor

jaymedina commented Jun 5, 2023

@bsipocz I agree there is a case for making sub-pages for the sections, rather than keeping everything in 1 page, to preserve vertical real estate. We'll be re-scoping the PR accordingly.

@mstcyr2 mstcyr2 closed this Jun 9, 2023
@mstcyr2 mstcyr2 reopened this Jun 12, 2023
@mstcyr2 mstcyr2 marked this pull request as ready for review July 17, 2023 14:11
Copy link
Contributor

@jaymedina jaymedina left a comment

Choose a reason for hiding this comment

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

This is looking good to me, I just had one small suggestion. I've sent this PR to Dick Shaw who would also like to give input on it, and I'll relay his comments later this week in a separate review.

@jaymedina
Copy link
Contributor

Dick Shaw has some comments that are slightly out of scope of this PR so we've agreed to tackle them in a separate one, and this one's good to go. I'll be wrapping up this PR for Makayla while she's on break, let me know if there's anything that needs to be updated before this is ready for merge @bsipocz. Thanks!

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.

Rendering looks OK, and I defer the content review to the MAST colleagues, so this is indeed good to go.

Thanks @mstcyr2!

@bsipocz
Copy link
Member

bsipocz commented Aug 9, 2023

so this is indeed good to go.

except that I see failing tests. Nevertheless, I would go ahead and merge this, but would like to ask to follow-up on the failures (they are already reported as part of #2801). Thanks to this PR, now it'll be more efficient to fix the issues as one doesn't need to rerun all of the docs over and over again.

@bsipocz bsipocz added this to the v0.4.7 milestone Aug 9, 2023
@bsipocz bsipocz merged commit 88dd76d into astropy:main Aug 9, 2023
@bsipocz
Copy link
Member

bsipocz commented Aug 9, 2023

FYI:

collected 6 items                                                                                                      

docs/mast/mast.rst .                                                                                             [ 16%]
docs/mast/mast_catalog.rst F                                                                                     [ 33%]
docs/mast/mast_cut.rst .                                                                                         [ 50%]
docs/mast/mast_mastquery.rst .                                                                                   [ 66%]
docs/mast/mast_missions.rst .                                                                                    [ 83%]
docs/mast/mast_obsquery.rst F                                                                                    [100%]

======================================================= FAILURES =======================================================
______________________________________________ [doctest] mast_catalog.rst ______________________________________________
254 
255 Given an HSC Match ID, return all catalog results.
256 
257 .. doctest-remote-data::
258 
259    >>> from astroquery.mast import Catalogs
260    ...
261    >>> catalog_data = Catalogs.query_object("M10", radius=.02, catalog="HSC")
262    >>> matchid = catalog_data[0]["MatchID"]
263    >>> print(matchid)
Expected:
    63980492
Got:
    7542452

/Users/bsipocz/munka/devel/astroquery/docs/mast/mast_catalog.rst:263: DocTestFailure
_____________________________________________ [doctest] mast_obsquery.rst ______________________________________________
049       science    SPITZER_SHA    SSC Pipeline ...    nan 19000016510      0.0
050       science    SPITZER_SHA    SSC Pipeline ...    nan 19000016510      0.0
051       science    SPITZER_SHA    SSC Pipeline ...    nan 19000016510      0.0
052 
053 Optional parameters must be labeled. For example the query above will produce
054 an error if the "radius" field is not specified.
055 
056 .. doctest-remote-data::
057 
058    >>> obs_table = Observations.query_object("M8", ".02 deg")
Differences (unified diff with -expected +actual):
    @@ -1,3 +1,12 @@
     Traceback (most recent call last):
    -...
    -TypeError: ObservationsClass.query_object_async() takes 2 positional arguments but 3 were given
    +  File "/Users/bsipocz/.pyenv/versions/3.9.1/lib/python3.9/doctest.py", line 1336, in __run
    +    exec(compile(example.source, filename, "single",
    +  File "<doctest mast_obsquery.rst[6]>", line 1, in <module>
    +    obs_table = Observations.query_object("M8", ".02 deg")
    +  File "/Users/bsipocz/munka/devel/astroquery/astroquery/utils/class_or_instance.py", line 25, in f
    +    return self.fn(obj, *args, **kwds)
    +  File "/Users/bsipocz/munka/devel/astroquery/astroquery/utils/process_asyncs.py", line 26, in newmethod
    +    response = getattr(self, async_method_name)(*args, **kwargs)
    +  File "/Users/bsipocz/munka/devel/astroquery/astroquery/utils/class_or_instance.py", line 25, in f
    +    return self.fn(obj, *args, **kwds)
    +TypeError: query_object_async() takes 2 positional arguments but 3 were given

/Users/bsipocz/munka/devel/astroquery/docs/mast/mast_obsquery.rst:58: DocTestFailure
=============================================== short test summary info ================================================
FAILED docs/mast/mast_catalog.rst::mast_catalog.rst
FAILED docs/mast/mast_obsquery.rst::mast_obsquery.rst
======================================= 2 failed, 4 passed in 642.32s (0:10:42) ========================================

@jaymedina jaymedina deleted the mast-docs-vr branch August 9, 2023 20:19
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.

3 participants