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

WIP: NRAO Archive Query replacement #3015

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

keflavich
Copy link
Contributor

The NRAO Archive API has changed completely, which forced us to remove the old one. We therefore asked for a new module.

This is a WIP based on the ALMA TAP query service (so far).

There are a few to-do items:

  • set up the correct parameters in NRAO_FORM_KEYS
  • test
  • hook up to SOLR-based data download windows (can't use TAP for this)

There's more than that, but I'm pushing this WIP so that @jjtobin can join in on this.

@pep8speaks
Copy link

pep8speaks commented May 29, 2024

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

Line 38:22: E128 continuation line under-indented for visual indent
Line 39:22: E128 continuation line under-indented for visual indent
Line 42:23: E231 missing whitespace after ','
Line 47:19: E241 multiple spaces after ','
Line 48:19: E241 multiple spaces after ','
Line 49:19: E241 multiple spaces after ','
Line 50:19: E241 multiple spaces after ','
Line 259:28: E127 continuation line over-indented for visual indent
Line 260:28: E127 continuation line over-indented for visual indent
Line 291:21: E127 continuation line over-indented for visual indent
Line 339:5: E303 too many blank lines (2)
Line 341:18: E124 closing bracket does not match visual indentation
Line 354:9: E265 block comment should start with '# '
Line 354:121: E501 line too long (166 > 120 characters)
Line 363:33: E124 closing bracket does not match visual indentation
Line 371:31: E124 closing bracket does not match visual indentation
Line 393:11: E121 continuation line under-indented for hanging indent
Line 411:36: E124 closing bracket does not match visual indentation
Line 422:30: E124 closing bracket does not match visual indentation
Line 440:121: E501 line too long (168 > 120 characters)
Line 454:1: E303 too many blank lines (3)

Line 43:1: E302 expected 2 blank lines, found 1
Line 146:13: W503 line break before binary operator
Line 254:121: E501 line too long (145 > 120 characters)
Line 259:1: E303 too many blank lines (3)

Comment last updated at 2024-06-02 15:09:36 UTC

@keflavich keflavich marked this pull request as draft May 29, 2024 20:34
@keflavich
Copy link
Contributor Author

This basic example works:

from astroquery.nrao import core
query="SELECT * FROM tap_schema.obscore WHERE CONTAINS(POINT('ICRS',s_ra,s_dec),CIRCLE('ICRS', 290.9583, 14.1, 0.01))=1"
result = core.Nrao.query_tap(query)
print(result)

@keflavich
Copy link
Contributor Author

I broke something though:

from astropy.coordinates import SkyCoord
from astropy import units as u, constants

crd = SkyCoord.from_name('w51')
result2 = core.Nrao.query_region_async(crd, 0.05*u.deg)
print(result2)
Traceback (most recent call last):
  File ~/Downloads/nrao_tap_test.py:14
    result2 = core.Nrao.query_region_async(crd, 0.05*u.deg)
  File ~/repos/astroquery/astroquery/nrao/core.py:220 in query_region_async
    return self.query_async(payload=payload, **kwargs)
  File ~/repos/astroquery/astroquery/nrao/core.py:262 in query_async
    result = self.query_tap(query, maxrec=maxrec)
  File ~/repos/astroquery/astroquery/nrao/core.py:189 in query_tap
    return self.tap.search(query, language='ADQL', maxrec=maxrec)
  File ~/repos/pyvo/pyvo/dal/tap.py:272 in run_sync
    **keywords).execute()
  File ~/repos/pyvo/pyvo/dal/tap.py:1106 in execute
    return TAPResults(self.execute_votable(), url=self.queryurl, session=self._session)
  File ~/repos/pyvo/pyvo/dal/adhoc.py:111 in __init__
    super().__init__(votable, url=url, session=session)
  File ~/repos/pyvo/pyvo/dal/query.py:322 in __init__
    raise DALQueryError(self._status[1], self._status[0], url)
DALQueryError: PSQLException: ERROR: operator does not exist: scircle && text
  Hint: No operator matches the given name and argument types. You might need to add explicit type casts.
  Position: 1270

Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 45.34884% with 94 lines in your changes are missing coverage. Please review.

Project coverage is 66.93%. Comparing base (462c2da) to head (076df46).
Report is 16 commits behind head on main.

Current head 076df46 differs from pull request most recent head 0ecb26d

Please upload reports for the commit 0ecb26d to get more accurate results.

Files Patch % Lines
astroquery/nrao/core.py 41.25% 94 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3015      +/-   ##
==========================================
- Coverage   67.35%   66.93%   -0.42%     
==========================================
  Files         236      233       -3     
  Lines       18320    18354      +34     
==========================================
- Hits        12339    12286      -53     
- Misses       5981     6068      +87     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jjtobin
Copy link

jjtobin commented May 29, 2024

It looks like the query is trying to use intersects with s_region,which ALMA data has, but VLA does not. If we wanted that column, we'd need to have that calculated and stored in the database.

So, for now, I think we need NRAO queries to use the format of the working query that you showed above.

@bsipocz
Copy link
Member

bsipocz commented Jun 1, 2024

I broke something though:

A few things:

  • there maybe corner cases with pyvo that may not work. If that's the case open bug reports upstream with minimal failing examples.
  • Do not bother with sync and async in a way that fits in the "old" astroquery setup. Most of the async stuff we have are not in fact real async, don't use the servers' async capabilities. Some discussion is here: API discussion: keeping duplicated methods xyz/xyz_async vs having an async kwarg? #2598, but bottom line, don't duplicate methods for new modules (and alma has them for historical reason at this point).

@bsipocz
Copy link
Member

bsipocz commented Jun 1, 2024

And while this is a WIP PR, it would be nice to keep a clean history, maybe even prepend commit messages for the temporary/debugging ones, and the style/fixing ones, so a later squash will be easier.

Useful tips on good commit messages are here, eventually I hope it will propagate into our devdocs: https://numpy.org/devdocs/dev/development_workflow.html#writing-the-commit-message

(additional commit message was just debug notes and should be ignored)

add back tapsql stuff

remove alma stuff from nrao

add nrao obscore thing

Adapt region queries to work with NRAO TAP service

add tapsql.py for nrao, updates to data columns supported by NRAO TAP

fix imports

implemented data retrieval code

flush

add VLA handling

fix the wait-until-done step

flex fixes
# TODO: this needs to be implemented
# """
# pass


class NraoClass(BaseQuery):
Copy link
Member

Choose a reason for hiding this comment

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

I believe you still want to inherit this from BaseVOQuery, so the session things passed through pyvo correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants