Skip to content

Esa documentation cleanup#1970

Merged
bsipocz merged 6 commits into
astropy:mainfrom
tinuademargaret:esa-doc-cleanup
Aug 17, 2022
Merged

Esa documentation cleanup#1970
bsipocz merged 6 commits into
astropy:mainfrom
tinuademargaret:esa-doc-cleanup

Conversation

@tinuademargaret

Copy link
Copy Markdown
Contributor

No description provided.

@ceb8 ceb8 force-pushed the esa-doc-cleanup branch from 7412146 to 4ca82d4 Compare July 21, 2022 15:21
@codecov

codecov Bot commented Aug 8, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1970 (5ef6eeb) into main (74fb75d) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 5ef6eeb differs from pull request most recent head 81f9ef2. Consider uploading reports for the commit 81f9ef2 to get more accurate results

@@            Coverage Diff             @@
##             main    #1970      +/-   ##
==========================================
- Coverage   62.92%   62.91%   -0.02%     
==========================================
  Files         133      133              
  Lines       17302    17307       +5     
==========================================
+ Hits        10888    10889       +1     
- Misses       6414     6418       +4     
Impacted Files Coverage Δ
astroquery/esa/hubble/core.py 86.47% <ø> (+0.35%) ⬆️
astroquery/astrometry_net/core.py 47.64% <0.00%> (-2.36%) ⬇️
astroquery/alma/core.py 43.35% <0.00%> (+0.55%) ⬆️

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

@ceb8 ceb8 marked this pull request as ready for review August 8, 2022 18:50
@bsipocz bsipocz added this to the v0.4.7 milestone Aug 8, 2022
@bsipocz

bsipocz commented Aug 8, 2022

Copy link
Copy Markdown
Member

@ceb8 - could you squash the 4th (file deleting commit) into the 3rd one, to avoid the files showing up in the history in the first place?

@ceb8 ceb8 force-pushed the esa-doc-cleanup branch from 22531b8 to ab35934 Compare August 10, 2022 19:45
@ceb8

ceb8 commented Aug 10, 2022

Copy link
Copy Markdown
Member

@ceb8 - could you squash the 4th (file deleting commit) into the 3rd one, to avoid the files showing up in the history in the first place?

Done.

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some comments that can potentially be addressed. However I would not hold the PR up from merging for everything to be addressed as tests already pass and this is already a nice improvement on the status quo.

Comment thread docs/esa/hsa/hsa.rst
------------------------

.. doctest-remote-data::
.. Skipping becuase of how long the download takes

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder whether it's possible to identify an observation_id that is much smaller, for here and below? cc @jespinosaar

Comment thread docs/esa/hubble/hubble.rst Outdated
Note: Artifact is a single Hubble product file.

.. code-block:: python
.. doctest-skip::

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why skip? add comment or run the code

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It doesn't run, I've marked it as such. @jespinosaar can you replace this with one that does run, or is the file in question just an example?

Comment thread docs/esa/iso/iso.rst Outdated
Comment thread docs/esa/iso/iso.rst Outdated
Comment thread docs/esa/jwst/jwst.rst

.. code-block:: python
.. Skipping becuase the download takes too long
.. doctest-skip::

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Again, maybe there is a dataset that is minimal at size? cc @jespinosaar

Comment thread docs/esa/xmm_newton/xmm_newton.rst
Comment thread docs/esa/xmm_newton/xmm_newton.rst
>>> from astroquery.esa.xmm_newton import XMMNewton
>>>
>>> XMMNewton.get_columns('public.v_all_observations')
>>> XMMNewton.get_columns('xsa.v_all_observations') # doctest: +IGNORE_OUTPUT

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same, I would rather keep the column list (my rule of thumb: ignore outputs when it's full of floats, and values that are e.g. a result of a truncated SQL query where it's not guaranteed that the same return will be given every time. But keep strings like these column names in for the tests)

@jespinosaar

Copy link
Copy Markdown
Contributor

Let me have a look at your comments @bsipocz, I will look for smaller datasets
cc @esdc-esac-esa-int

@bsipocz

bsipocz commented Aug 17, 2022

Copy link
Copy Markdown
Member

OK, so I did one minor cleanup, and as this passes the remote test, going ahead and merge it. I'll open a follow-up issue for using smaller datasets if possible to clean up the remaining example skips.

Thanks @ceb8!

@ceb8 ceb8 mentioned this pull request Sep 8, 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.

4 participants