BUG: raise for unsupported arguments for alma

[at88mph]

Throw TypeError for unused arguments when generating query.

[keflavich]

Looks like a great solution!

One minor nitpick: I think the convention is to use ValueError for incorrect arguments, not TypeError.

[codecov]

Codecov Report

Merging #2475 (59ff73e) into main (b1fcfff) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2475      +/-   ##
==========================================
- Coverage   62.92%   62.91%   -0.02%     
==========================================
  Files         133      133              
  Lines       17302    17313      +11     
==========================================
+ Hits        10888    10893       +5     
- Misses       6414     6420       +6     

Impacted Files	Coverage Δ
astroquery/alma/core.py	43.35% <100.00%> (+0.55%)	⬆️
astroquery/astrometry_net/core.py	48.46% <0.00%> (-1.54%)	⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[keflavich]

You could also opt to warn with an InputWarning. We don't have any more specific exceptions, though.

[at88mph]

Happily rectified. Thank you @keflavich.

[keflavich]

Suggested change

	with patch('astroquery.alma.tapsql.coord.SkyCoord.from_name') as name_mock, pytest.raises(ValueError) as typeError:
	name_mock.return_value = SkyCoord(1, 2, unit='deg')
	alma.query_object('M13', public=False, bogus=True, nope=False, band_list=[3])
	assert "['bogus -> True', 'nope -> False']" in str(typeError.value)
	with patch('astroquery.alma.tapsql.coord.SkyCoord.from_name') as name_mock, pytest.raises(ValueError) as valueError:
	name_mock.return_value = SkyCoord(1, 2, unit='deg')
	alma.query_object('M13', public=False, bogus=True, nope=False, band_list=[3])
	assert "['bogus -> True', 'nope -> False']" in str(valueError.value)

[keflavich]

totally minor, but may as well have the variable names match the objects

[at88mph]

Right, oops. Done.

[eerovaher]

One minor nitpick: I think the convention is to use ValueError for incorrect arguments, not TypeError.

Incorrect argument should raise a TypeError, for example:

>>> print(bad_argument=0)
Traceback (most recent call last):
  ...
TypeError: 'bad_argument' is an invalid keyword argument for print()

[keflavich]

Oh, my bad. Sorry @at88mph , revert my suggestions.

[at88mph]

Done, thanks.

[bsipocz]

I wonder how many things can be shuffled into **kwargs in the first place, and therefore couldn't we actually address the route cause of this bug by spelling out those optional keyword arguments rather than hiding them in **kwargs instead?

[bsipocz]

This will need a changelog before the release.

This PR looks good for doing a quick fix for the issue and thus I'm OK with merging, but honestly I would rather see all **kwargs gone from the codebase instead. Would you think it's possible?

[bsipocz]

Actually, I have to retract my approval as some remote-data tests are failing with this PR. They are more like the case of wrong tests where extra arguments were passed along that this fix has recovered, yet they should be fixed before merging this.

I currently see these failures, but some are already present on the main branch, and only some are due to changes here.

FAILED astroquery/alma/tests/test_alma_remote.py::TestAlma::test_public - TypeError: Unsupported arguments were passed:
FAILED astroquery/alma/tests/test_alma_remote.py::TestAlma::test_data_proprietary - AttributeError: uid://A001/X12a3/Xe9 not found
FAILED astroquery/alma/tests/test_alma_remote.py::TestAlma::test_data_info - assert 9 > 9
FAILED astroquery/alma/tests/test_alma_remote.py::TestAlma::test_doc_example - KeyError: 'size'
FAILED astroquery/alma/tests/test_alma_remote.py::TestAlma::test_misc - TypeError: Unsupported arguments were passed:

[at88mph]

I see this now, thanks @bsipocz. 😵‍💫

The kwargs are getting in the way here, but maxrec is a valid request parameter, but not a valid constraint, which is the difference. Very annoying. Let me look at it closer.

[at88mph]

I've added maxrec as a function argument which is consistent with other functions. I've also added a CHANGES.rst entry.

[bsipocz]

One more to go:

self = <astroquery.alma.tests.test_alma_remote.TestAlma object at 0x113b6db40>, alma = <astroquery.alma.core.AlmaClass object at 0x114fb5540>

    def test_misc(self, alma):
        # miscellaneous set of common tests
        #
        # alma.query_region(coordinate=orionkl_coords, radius=4 * u.arcmin,
        #                  public=False, science=False)
    
        result = alma.query_object('M83', public=True, science=True)
        assert len(result) > 0
        result = alma.query(payload={'pi_name': '*Bally*'}, public=False,
                            maxrec=10)
        assert result
        result.write('/tmp/alma-onerow.txt', format='ascii')
        for row in result:
            assert 'Bally' in row['obs_creator_name']
>       result = alma.query(payload=dict(project_code='2016.1.00165.S'),
                            public=False, cache=False)

astroquery/alma/tests/test_alma_remote.py:319: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
astroquery/utils/class_or_instance.py:25: in f
    return self.fn(obj, *args, **kwds)
astroquery/utils/process_asyncs.py:26: in newmethod
    response = getattr(self, async_method_name)(*args, **kwargs)
astroquery/alma/core.py:360: in query_async
    query = _gen_sql(payload)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

payload = {'cache': False, 'project_code': '2016.1.00165.S', 'public_data': False, 'science_observation': True}

    def _gen_sql(payload):
        sql = 'select * from ivoa.obscore'
        where = ''
        unused_payload = payload.copy()
        if payload:
            for constraint in payload:
                for attrib_category in ALMA_FORM_KEYS.values():
                    for attrib in attrib_category.values():
                        if constraint in attrib:
                            # use the value and the second entry in attrib which
                            # is the new name of the column
                            val = payload[constraint]
                            if constraint == 'em_resolution':
                                # em_resolution does not require any transformation
                                attrib_where = _gen_numeric_sql(constraint, val)
                            else:
                                attrib_where = attrib[2](attrib[1], val)
                            if attrib_where:
                                if where:
                                    where += ' AND '
                                else:
                                    where = ' WHERE '
                                where += attrib_where
    
                            # Delete this key to see what's left over afterward
                            #
                            # Use pop to avoid the slight possibility of trying to remove
                            # an already removed key
                            unused_payload.pop(constraint)
    
        if unused_payload:
            # Left over (unused) constraints passed.  Let the user know.
            remaining = [f'{p} -> {unused_payload[p]}' for p in unused_payload]
>           raise TypeError(f'Unsupported arguments were passed:\n{remaining}')
E           TypeError: Unsupported arguments were passed:
E           ['cache -> False']

astroquery/alma/core.py:195: TypeError

[at88mph]

This was a bit of a rabbit hole.

Running on Python 3.10:

1. The TestAlma.test_bands function was dying silently, but appeared to succeed, and thus I thought all tests were passing. The truth is that the result set was too long to read which caused it to just die.
2. The latest DataLink service is released but the tests aren't caught up until PR2438 is merged. I've had to ignore TestAlma.test_data_info for now because of this.
3. The obs_id column in the ObsCore TAP service's value has changed slightly to more accurately represent the underlying data, so some of the queries had to be altered.

I hope this change hasn't damaged anything that I don't know about.

[bsipocz]

Thank you, and extra thanks for fixing/working around tests that are failing unrelatedly to this PR.