Use TAP with alma - initial version #1689

andamian · 2020-03-31T09:23:15Z

No description provided.

keflavich · 2020-03-31T13:34:59Z

astroquery/alma/core.py

+    'gal_longitude': 'Galactic longitude',
+    'gal_latitude':	'Galactic latitude',
+    'band_list': 'Band',
+    's_region':	'Spatial resolution',


could use some whitespace cleanup

keflavich · 2020-03-31T13:35:08Z

astroquery/alma/core.py

+try:
+    import pyvo
+except ImportError:
+    print('Please install pyvo. astropy.vo does not work without it.')


as w/previous: shouldn't this just raise?

keflavich · 2020-03-31T13:35:19Z

astroquery/alma/core.py

+}
+
+ALMA_BANDS = {
+    '3': (84*10**9*u.Hz, 116*10**9*u.Hz),


you don't like GHz?

keflavich · 2020-03-31T13:36:19Z

astroquery/alma/core.py

+        # galactic (Galactic longitude, Galactic latitude str) or
+        # source_name_resolver (object name) or combination.
+        # Add them all to the pos list
+        radius = payload.get('radius', 0.016666666666667*u.deg)


maybe 1*u.arcmin?

keflavich · 2020-04-03T01:31:53Z

To give you a sense of the API as I've used it, here's a list of a bunch of different queries I have run:

Alma.query_region(coordinate=orionkl_coords, radius=4*u.arcmin, public=False, science=False)
Alma.query_object('M83', public=False, science=False)
Alma.query(payload={'pi_name':'Bally'}, public=False)
alma.query(payload=dict(project_code='2016.1.00165.S'), public=False, cache=False)
alma.query(payload=dict(project_code='2017.1.01355.L', source_name_alma='G008.67'),)
Alma.query_region(coordinates.SkyCoord('5:35:14.461 -5:21:54.41', frame='fk5', unit=(u.hour, u.deg)), radius=0.034*u.deg)
Alma.query_region(coordinates.SkyCoord('5:35:14.461 -5:21:54.41', frame='fk5', unit=(u.hour, u.deg)), radius=0.034*u.deg, payload={'energy.frequency-asu':'215 .. 220'})
Alma.query(payload=dict(project_code='2012.*', public_data=True))
Alma.query(payload={'frequency':'96 .. 96.5'}, cache=False)
rslt = Alma.query_object('M83', band_list=[4,5,8])
Alma.query(payload={'pi_name':'ginsburg', 'band_list':'6'})

but I'll note that my history of using the band & frequency queries is littered with dozens of failed attempts to get the syntax right. The API we'd like to support does not have to take the same syntax, and in fact it would be helpful if we could check the syntax (and specify it) user-side.

So for the basic query API, as long as we continue to support the basic region queries and some variants of the criteria query (on PI, project ID, frequency, band, polarization [tricky, I know], etc), it's OK. I think we'll have to live with syntax-breaking in the uploaded values (e.g., 96 .. 96.5 above) because we don't really know what the old syntax was anyway.

andamian · 2020-04-07T02:39:01Z

The majority of those examples should work with the changes in this PR (and probably a few others). Wild cards (project_code='2012.*') however do not work.
That being said I suggest the following:
Have query interface for general users and one for advanced ALMA users. Thi,. in fact, is Simple Image Access and TAP Obscore protocols' raison d'être.
More concrete, I suggest exposing the SIA attributes in the query* methods. Issue deprecate warnings for payload arguments but for now try to have them still work by "translating" them into SIA constraints whenever possible. Same deal for the results. This will constitute the general user ALMA API.

For users with more intimate knowledge about ALMA, we can create a method to query the TAP service. Users will be given tools to explore the schema of the TAP service (help_tap) and write their own queries. That's where they can use wildcards, sorting, grouping and all the other goodies.

This approach is not too complicated, keeps the library simple, tries to make the changes less disruptive and appeals to any type of audience, including power users.

In summary, I'm proposing:

add sia argument to query* as a dictionary with the following expected keys: pos, band, time, pol, field_of_view, spatial_resolution, spectral_resolving_power, exptime, timeres, publisher_did, facility, collection, instrument, data_type, calib_level, target_name, res_format, maxrec. Or should they be exposed directly as method arguments the way I've proposed it in the vo module?
Add deprecate warnings when payload argument is detected but attempt to perform operations when possible.
Add help_sia method. Also update help method to explain that the interface is deprecated and which arguments are still supported in the current version.

Add query_tap method to query ObsCore via TAP.
Add help_tap to query ALMA TAP schema and return information regarding the available ObsCore columns and their types.

Sounds acceptable?

keflavich · 2020-04-07T13:40:02Z

I like this plan.

Re: (1) - let's have these as options to query_*. I don't immediately understand how they would work as methods, but we want to continue supporting query* interfaces since the rest of astroquery has them.

Everything else is fine.

andamian · 2020-04-07T21:00:39Z

I like this plan.

Re: (1) - let's have these as options to query_*. I don't immediately understand how they would work as methods, but we want to continue supporting query* interfaces since the rest of astroquery has them.

Everything else is fine.

What I meant is that query* can either have all those arguments add with defaults or use a single sia generic dictionary argument (similar to payload). The former is a bit better because works with autocompletion and IDEs, while the latter is consistent with the original payload (generic dictionary argument + corresponding help method).

keflavich · 2020-04-07T21:23:00Z

yeah, I like the IDE-friendlier specific arguments approach.

andamian · 2020-04-09T22:31:54Z

I'm adding to the thread the link to the google docs used for mapping between old parameters and the new ones. Needs to be reviewed and commented on.
https://docs.google.com/spreadsheets/d/1X0rrHuTMtU4Tp_CnBR1CdylffvNUIliTgdNMhnLCCws/edit#gid=0

andamian · 2020-04-16T03:39:32Z

The query samples you've posted with the new API:

        alma = Alma()
        #alma.query_region(coordinate=orionkl_coords, radius=4 * u.arcmin,
        #                  public=False, science=False)
        result = alma.query_object('M83', public=False, science=False)
        assert len(result) > 0
        #TODO test public
        result = alma.query_tap(
            "select * from ivoa.ObsCore where obs_creator_name like '%Bally%'")
        assert result
        for row in result:
            assert 'Bally' in row['obs_creator_name']
        result = alma.query_tap\
            ("select * from ivoa.Obscore where proposal_id='2016.1.00165.S'")
        assert result
        for row in result:
            assert '2016.1.00165.S' == row['proposal_id']
        result = alma.query_tap(
            "select * from ivoa.Obscore where proposal_id='2017.1.01355.L' "
            "and target_name like '%G008.67%' and data_rights='Public'")
        assert result
        for row in result:
            assert '2017.1.01355.L' == row['proposal_id']
            assert 'Public' == row['data_rights']
            assert 'G008.67'in row['target_name']

        result = alma.query_region(
            coordinates.SkyCoord('5:35:14.461 -5:21:54.41', frame='fk5',
                                 unit=(u.hour, u.deg)), radius=0.034 * u.deg)
        assert result

        # result = alma.query_region(
        #     coordinates.SkyCoord('5:35:14.461 -5:21:54.41', frame='fk5',
        #                          unit=(u.hour, u.deg)), radius=0.034 * u.deg,
        #     payload={'energy.frequency-asu': '215 .. 220'})
        #
        result = alma.query_tap("select * from ivoa.ObsCore where proposal_id "
                                "like '2012.%' and data_rights='Public'")
        assert result
        for row in result:
            assert '2012.' in row['proposal_id']
            assert 'Public' == row['data_rights']

        result = alma.query(payload={'frequency': '96 .. 96.5'})
        assert result
        for row in result:
            # TODO not sure how to test this
            pass

        result = alma.query_object('M83', band_list=[4, 5, 8])
        assert result
        for row in result:
            assert row['band_list'] in ['4', '5', '8']

        result = alma.query_tap(
            "select * from ivoa.ObsCore where lower(obs_creator_name) like "
            "'%ginsburg%' and band_list = '6'")
        assert result
        for row in result:
            assert '6' == row['band_list']
            assert 'ginsburg' in row['obs_creator_name'].lower()

Looks OK? I didn't know what energy.frequency-asu is or how to find it in ALMA ObsCore.

keflavich · 2020-04-16T14:22:36Z

Yes, looks good, though I definitely want a shortcut approach to those SQL queries!

I don't know what energy.frequency-asu is either. It looks like I was just trying to find a way to search within a frequency range.

You can put some of this stuff in as actual tests and I can give you some inline comments

pep8speaks · 2020-05-25T04:55:51Z

Hello @andamian! 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 2020-07-29 18:27:58 UTC

codecov · 2020-06-29T03:32:32Z

Codecov Report

Merging #1689 into master will increase coverage by 0.27%.
The diff coverage is 79.35%.

@@            Coverage Diff             @@
##           master    #1689      +/-   ##
==========================================
+ Coverage   63.63%   63.91%   +0.27%     
==========================================
  Files         199      200       +1     
  Lines       15543    15729     +186     
==========================================
+ Hits         9891    10053     +162     
- Misses       5652     5676      +24

Impacted Files	Coverage Δ
astroquery/alma/utils.py	`29.22% <11.11%> (+0.08%)`	⬆️
astroquery/alma/core.py	`34.11% <66.23%> (-1.32%)`	⬇️
astroquery/alma/tapsql.py	`94.28% <94.28%> (ø)`
astroquery/alma/__init__.py	`100.00% <100.00%> (ø)`

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 6c79ac4...bf3a7fb. Read the comment docs.

keflavich · 2020-07-25T20:21:59Z

We've been given a deadline of "this Wednesday", after which the present ALMA archive will be disabled.

There are problems.

All queries were failing with:

rslt = Alma.query_object('W51', pi_name='*Ginsburg*', public=False)
Traceback (most recent call last):
  File "<ipython-input-12-dbcc322179c1>", line 1, in <module>
    rslt = Alma.query_object('W51', pi_name='*Ginsburg*', public=False)
  File "/home/adam/repos/astroquery/astroquery/utils/class_or_instance.py", line 25, in f
    return self.fn(obj, *args, **kwds)
  File "/home/adam/repos/astroquery/astroquery/utils/process_asyncs.py", line 26, in newmethod
    response = getattr(self, async_method_name)(*args, **kwargs)
  File "/home/adam/repos/astroquery/astroquery/alma/core.py", line 245, in query_object_async
    payload=payload, **kwargs)
  File "/home/adam/repos/astroquery/astroquery/alma/core.py", line 343, in query_async
    result[_OBSCORE_TO_ALMARESULT[ii]] = result[ii]
  File "/home/adam/repos/astropy/astropy/table/table.py", line 1648, in __getitem__
    return self.columns[item]
  File "/home/adam/repos/astropy/astropy/table/table.py", line 239, in __getitem__
    return OrderedDict.__getitem__(self, item)
KeyError: 'frequency'

which resulted from a bad mapping..

keflavich · 2020-07-25T20:23:18Z

astroquery/alma/core.py

+                for _ in result]
+        for ii in _OBSCORE_TO_ALMARESULT:
+            if _OBSCORE_TO_ALMARESULT[ii] not in result.columns:
+                # duplicate equivalent columns


why are we doing this? For backward compatibility? I don't like it, I'd rather toss one or the other. I guess we can toss the old, stick with the new. We should add, though, a renaming utility using the _OBSCORE_TO_ALMARESULT mapping to allow users to easily recover the old naming scheme.

keflavich · 2020-07-25T20:28:33Z

astroquery/alma/tests/test_alma.py

+    row = result[0]
+    for item in _OBSCORE_TO_ALMARESULT.items():
+        if item[0] == 't_min':
+            assert Time(row[item[0]], format='mjd').strftime('%d-%m-%Y') ==\


my changes cause a failure here, but I suspect that's because the data are out of date - alma_onerow contains data that aren't returned from alma at present.

keflavich · 2020-07-25T20:29:25Z

astroquery/alma/core.py

-        cache : bool
-            Cache the query?
+            The object name.  Will be resolved by astropy.coord.SkyCoord
+        cache : deprecated


is it necessary to deprecate caching? Does pyvo not support it? The caching functionality is extremely useful, even though it is problematic in several ways (we have no way to test if the cache is out of date, for instance)

keflavich · 2020-07-25T20:37:33Z

astroquery/alma/core.py

+    'is_mosaic': 'Mosaic',
+    't_exptime': 'Integration',
+    'obs_release_date': 'Release date',
+    'frequency': 'Frequency support',


I changed this & coi_name, but that triggered some remote test failures. My changes may therefore be incorrect and need to be reverted - feel free to do so, but let's make sure the docs examples work.

CHANGES.rst

ceb8

Once the extra changes leaking into the PR issues has been resolved I approve this PR. Looks good.

bsipocz · 2020-08-06T17:58:45Z

Thanks @andamian!

astropy-bot bot added the alma label Mar 31, 2020

keflavich reviewed Mar 31, 2020

View reviewed changes

andamian changed the title ~~Used SIAv2 with alma - initial version~~ Used TAP with alma - initial version May 25, 2020

andamian changed the title ~~Used TAP with alma - initial version~~ Use TAP with alma - initial version May 25, 2020

keflavich mentioned this pull request Jun 8, 2020

alma.stage_data : isProprietary - always True ? #1730

Closed

andamian force-pushed the alma branch 3 times, most recently from 4aa6534 to 6982b5f Compare June 29, 2020 03:12

andamian force-pushed the alma branch from 6982b5f to e54d334 Compare July 13, 2020 16:05

keflavich force-pushed the alma branch from 59bedaa to 48d87b3 Compare July 25, 2020 20:10

keflavich reviewed Jul 25, 2020

View reviewed changes

andamian force-pushed the alma branch 3 times, most recently from 645cca3 to 6518c9d Compare July 29, 2020 04:36

keflavich reviewed Jul 29, 2020

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

ceb8 self-requested a review July 29, 2020 16:50

ceb8 approved these changes Jul 29, 2020

View reviewed changes

andamian force-pushed the alma branch 2 times, most recently from 3bc127c to a1e5fcc Compare July 29, 2020 17:35

Re-factored alma to use the IVOA TAP service from pyvo for querying.

bf3a7fb

andamian force-pushed the alma branch from 2ea727e to bf3a7fb Compare July 29, 2020 18:27

bsipocz added this to the v0.4.2 milestone Jul 31, 2020

keflavich merged commit 2fe796d into astropy:master Aug 6, 2020

andamian deleted the alma branch August 16, 2020 18:01

bsipocz mentioned this pull request Nov 6, 2020

'public=True' in Alma.query() returns more results #1888

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use TAP with alma - initial version #1689

Use TAP with alma - initial version #1689

andamian commented Mar 31, 2020

keflavich Mar 31, 2020

keflavich Mar 31, 2020

keflavich Mar 31, 2020

keflavich Mar 31, 2020

keflavich commented Apr 3, 2020

andamian commented Apr 7, 2020

keflavich commented Apr 7, 2020

andamian commented Apr 7, 2020

keflavich commented Apr 7, 2020

andamian commented Apr 9, 2020

andamian commented Apr 16, 2020

keflavich commented Apr 16, 2020

pep8speaks commented May 25, 2020 •

edited

codecov bot commented Jun 29, 2020 •

edited

keflavich commented Jul 25, 2020

keflavich Jul 25, 2020

keflavich Jul 25, 2020

keflavich Jul 25, 2020

keflavich Jul 25, 2020

ceb8 left a comment

bsipocz commented Aug 6, 2020

Use TAP with alma - initial version #1689

Use TAP with alma - initial version #1689

Conversation

andamian commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keflavich commented Apr 3, 2020

andamian commented Apr 7, 2020

keflavich commented Apr 7, 2020

andamian commented Apr 7, 2020

keflavich commented Apr 7, 2020

andamian commented Apr 9, 2020

andamian commented Apr 16, 2020

keflavich commented Apr 16, 2020

pep8speaks commented May 25, 2020 • edited

Comment last updated at 2020-07-29 18:27:58 UTC

codecov bot commented Jun 29, 2020 • edited

Codecov Report

keflavich commented Jul 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceb8 left a comment

Choose a reason for hiding this comment

bsipocz commented Aug 6, 2020

pep8speaks commented May 25, 2020 •

edited

codecov bot commented Jun 29, 2020 •

edited