Skip to content

Allow retrieval from a previous ESO archive request#1614

Merged
bsipocz merged 4 commits into
astropy:mainfrom
gbrammer:master
Nov 24, 2021
Merged

Allow retrieval from a previous ESO archive request#1614
bsipocz merged 4 commits into
astropy:mainfrom
gbrammer:master

Conversation

@gbrammer

Copy link
Copy Markdown
Contributor

Requesting large numbers of datasets from the ESO archive can take a long time to execute and this provides the opportunity to avoid duplicating an earlier request that my have timed out or otherwise broken.

The changes here allow a user to manually specify an ESO Archive request ID in eso.retrieve_data rather than always sending a new retrieval request. The desired request_id could either have been generated internally to astroquery or manually.

Here we do check that the specified retrieval URL based in the request_id is valid and then that the datasets indicated there are consistent with the user-specified datasets, but there is currently no reverse checking that the specified datasets are provided in request_id.

Generally, the user can identify valid request_ids either from the confirmation emails sent from the archive related to the request or at https://dataportal.eso.org/rh/requests/[USERNAME]/recentRequests.

@pep8speaks

pep8speaks commented Jan 16, 2020

Copy link
Copy Markdown

Hello @gbrammer! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-11-24 13:49:57 UTC

@codecov

codecov Bot commented Jan 16, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1614 (0ecad92) into main (e8489d3) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1614      +/-   ##
==========================================
- Coverage   61.96%   61.91%   -0.05%     
==========================================
  Files         129      129              
  Lines       16297    16310      +13     
==========================================
  Hits        10099    10099              
- Misses       6198     6211      +13     
Impacted Files Coverage Δ
astroquery/eso/core.py 29.09% <0.00%> (-0.71%) ⬇️

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 e8489d3...0ecad92. Read the comment docs.

@keflavich keflavich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a great modification. I have a few request:

  1. small fix to f-string (see inline)
  2. add a changelog entry (this is important and should be advertised)
  3. mention the change in the docs

Comment thread astroquery/eso/core.py
@gbrammer

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback @keflavich . Requested changes submitted.

@keflavich

Copy link
Copy Markdown
Contributor

Thanks for the fixes, and sorry about my delay in responding. The present failure is because, in the docs, you've used single-backticks around variable names. If you change those to double-backticks, it should fix the build, then I'm ready to merge.

Comment thread docs/eso/eso.rst Outdated
@keflavich

Copy link
Copy Markdown
Contributor

To save you scrolling through the Travis logs, here are the errors:

/home/travis/build/astropy/astroquery/build/lib.linux-x86_64-3.7/astroquery/eso/core.py:docstring of astroquery.eso.EsoClass.retrieve_data:32: WARNING: py:obj reference target not found: request_id
/home/travis/build/astropy/astroquery/docs/eso/eso.rst:321: WARNING: py:obj reference target not found: eso.retrieve_data
/home/travis/build/astropy/astroquery/docs/eso/eso.rst:321: WARNING: py:obj reference target not found: datasets
/home/travis/build/astropy/astroquery/docs/eso/eso.rst:321: WARNING: py:obj reference target not found: retrieve_data
/home/travis/build/astropy/astroquery/docs/eso/eso.rst:321: WARNING: py:obj reference target not found: request_id
/home/travis/build/astropy/astroquery/docs/eso/eso.rst:331: WARNING: py:obj reference target not found: request_id
/home/travis/build/astropy/astroquery/docs/eso/eso.rst:331: WARNING: py:obj reference target not found: https://dataportal.eso.org/rh/requests/[USERNAME]/{request_id}
/home/travis/build/astropy/astroquery/docs/eso/eso.rst:331: WARNING: py:obj reference target not found: https://dataportal.eso.org/rh/requests/[USERNAME]/recentRequests
/home/travis/build/astropy/astroquery/docs/eso/eso.rst:334: WARNING: py:obj reference target not found: request_id
/home/travis/build/astropy/astroquery/docs/eso/eso.rst:334: WARNING: py:obj reference target not found: datasets
/home/travis/build/astropy/astroquery/docs/eso/eso.rst:334: WARNING: py:obj reference target not found: datasets
/home/travis/build/astropy/astroquery/docs/eso/eso.rst:334: WARNING: py:obj reference target not found: request_id

@bsipocz bsipocz added the eso label Jan 22, 2020
@bsipocz bsipocz added this to the v4 milestone Jan 23, 2020
@keflavich

Copy link
Copy Markdown
Contributor

@gbrammer This PR is in a pretty good state and was nearly ready to merge. Are you able to do the last little bits of cleanup, and a rebase, so we can get this into astroquery?

@gbrammer

Copy link
Copy Markdown
Contributor Author

@kevlavich I think I have the changes implemented as requested. I'm not sure what the astropy-bot:changelog error means, though. I moved the relevant item from the 0.4 section up to 0.4.1 (unreleased).

@bsipocz

bsipocz commented Jun 12, 2020

Copy link
Copy Markdown
Member

I've rebased this to get rid of the merge commit, etc.

@gbrammer

Copy link
Copy Markdown
Contributor Author

@keflavich Is there a way I can fix the codecov failures on this PR?

@keflavich

Copy link
Copy Markdown
Contributor

You could add a test for the new functionality. Do you have an idea how you'd do that?

@bsipocz bsipocz modified the milestones: v0.4.1, v0.4.2 Jun 18, 2020
@bsipocz bsipocz modified the milestones: v0.4.2, v0.4.3 May 15, 2021
@bsipocz bsipocz modified the milestones: v0.4.3, v0.4.4 Jul 7, 2021
@bsipocz bsipocz removed this from the v0.4.4 milestone Nov 13, 2021
@bsipocz

bsipocz commented Nov 24, 2021

Copy link
Copy Markdown
Member

I've rebased this and squashed, we should merge if it's all green.

@bsipocz bsipocz dismissed keflavich’s stale review November 24, 2021 13:51

Review has been addressed. One review point, about f-string is outdated now, so I have squashed out the commit that addressed it.

@bsipocz bsipocz added this to the v0.4.5 milestone Nov 24, 2021
@bsipocz bsipocz merged commit c8d678b into astropy:main Nov 24, 2021
@bsipocz

bsipocz commented Nov 24, 2021

Copy link
Copy Markdown
Member

Thank you @gbrammer!

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