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

Esa documentation cleanup #1970

Merged
merged 6 commits into from
Aug 17, 2022
Merged

Conversation

tinuademargaret
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Aug 8, 2022

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
Copy link
Member

bsipocz commented Aug 8, 2022

@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
Copy link
Member

ceb8 commented Aug 10, 2022

@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.

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.

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.

@@ -20,15 +20,17 @@ Examples
1. Getting Herschel data
------------------------

.. doctest-remote-data::
.. Skipping becuase of how long the download takes
Copy link
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

docs/esa/hubble/hubble.rst Outdated Show resolved Hide resolved
@@ -82,11 +83,11 @@ Calibration levels can be RAW, CALIBRATED, PRODUCT or AUXILIARY.

Note: Artifact is a single Hubble product file.

.. code-block:: python
.. 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 skip? add comment or run the code

Copy link
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?

docs/esa/iso/iso.rst Outdated Show resolved Hide resolved
docs/esa/iso/iso.rst Outdated Show resolved Hide resolved
docs/esa/jwst/jwst.rst Show resolved Hide resolved
@@ -23,15 +21,18 @@ Examples
1. Getting XMM-Newton data
--------------------------

.. code-block:: python
.. Skipping becuase the download takes too long
.. 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.

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

docs/esa/xmm_newton/xmm_newton.rst Show resolved Hide resolved
docs/esa/xmm_newton/xmm_newton.rst Show resolved Hide resolved

>>> 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
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
Contributor

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

@bsipocz
Copy link
Member

bsipocz commented Aug 17, 2022

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.

None yet

4 participants