MAINT: moving the str input validation out to avoid astropy deprecation

[bsipocz]

This fixes the branch new astropy deprecation warnings. We only had the string input checked for cases where it should raise an exception, but there could be some cases where it's part of a valid input.

@andamian wrote the original code, so I would like to hear if he is OK with this workaround or have something else in mind instead (I mean this is not a nice workaround).

_________________________________________________ test_pos _________________________________________________

    def test_pos():
        class TestClass(dict, AxisParamMixin):
            pass
        test_obj = TestClass()
        test_obj.pos.add((1, 2, 3) * u.deg)
        assert len(test_obj._pos) == 1
        assert test_obj['POS'] == ['CIRCLE 1.0 2.0 3.0']
    
        test_obj.pos.add((1, 2, 3, 4))
        assert len(test_obj._pos) == 2
        assert test_obj['POS'] == ['CIRCLE 1.0 2.0 3.0', 'RANGE 1.0 2.0 3.0 4.0']
    
        # duplicates are ignored
        test_obj.pos.add((1, 2, 3))
        assert len(test_obj._pos) == 2
        assert test_obj['POS'] == ['CIRCLE 1.0 2.0 3.0', 'RANGE 1.0 2.0 3.0 4.0']
    
        # polygon
        test_obj.pos.add((1, 2, 3, 4, 5, 6))
        assert len(test_obj._pos) == 3
        assert test_obj['POS'] == ['CIRCLE 1.0 2.0 3.0', 'RANGE 1.0 2.0 3.0 4.0',
                                   'POLYGON 1.0 2.0 3.0 4.0 5.0 6.0']
    
        # deletes
        test_obj.pos.remove((1, 2, 3, 4))
        assert len(test_obj._pos) == 2
        assert test_obj['POS'] == ['CIRCLE 1.0 2.0 3.0',
                                   'POLYGON 1.0 2.0 3.0 4.0 5.0 6.0']
    
        # test borders
        test_obj.pos.discard((1, 2, 3) * u.deg)
        test_obj.pos.discard((1, 2, 3, 4, 5, 6))
        assert (len(test_obj._pos) == 0)
        test_obj.pos.add((0, 90, 90))
        assert len(test_obj._pos) == 1
        assert test_obj['POS'] == ['CIRCLE 0.0 90.0 90.0']
        test_obj.pos.pop()
        test_obj.pos.add((360, -90, 1))
        assert len(test_obj._pos) == 1
        assert test_obj['POS'] == ['CIRCLE 360.0 -90.0 1.0']
        test_obj.pos.pop()
        test_obj.pos.add((0, 360, -90, 90))
        assert len(test_obj._pos) == 1
        assert test_obj['POS'] == ['RANGE 0.0 360.0 -90.0 90.0']
        test_obj.pos.pop()
        test_obj.pos.add((0, 0, 180, 90, 270, -90))
        assert len(test_obj._pos) == 1
        assert test_obj['POS'] == ['POLYGON 0.0 0.0 180.0 90.0 270.0 -90.0']
    
        # errors
        test_obj.pos.pop()
        with pytest.raises(ValueError):
>           test_obj.pos.add(('A', 2, 3))

pyvo/dal/tests/test_adhoc.py:66: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
pyvo/dal/params.py:259: in add
    if item in self:
pyvo/dal/params.py:280: in __contains__
    return self.get_dal_format(item) in self.dal
pyvo/dal/params.py:304: in get_dal_format
    self._validate_pos(val)
pyvo/dal/params.py:337: in _validate_pos
    self._validate_ra(pos[0])
pyvo/dal/params.py:371: in _validate_ra
    ra = ra * u.deg
../astropy/astropy/units/core.py:861: in __rmul__
    self._warn_about_operation_with_deprecated_type("products", m)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

op = 'products', other = 'A'

    @staticmethod
    def _warn_about_operation_with_deprecated_type(op: str, other: bytes | str) -> None:
>       warnings.warn(
            AstropyDeprecationWarning(
                f"{op} involving a unit and a '{type(other).__name__}' instance are "
                f"deprecated since v7.1. Convert {other!r} to a unit explicitly."
            ),
            stacklevel=3,
        )
E       astropy.utils.exceptions.AstropyDeprecationWarning: products involving a unit and a 'str' instance are deprecated since v7.1. Convert 'A' to a unit explicitly.

../astropy/astropy/units/core.py:791: AstropyDeprecationWarning

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.30%. Comparing base (a9ac860) to head (d1fa2bb).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #636      +/-   ##
==========================================
- Coverage   82.31%   82.30%   -0.01%     
==========================================
  Files          72       72              
  Lines        7429     7427       -2     
==========================================
- Hits         6115     6113       -2     
  Misses       1314     1314              

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

[mhvk]

A few comments, feeling responsible for the problems I caused!

[mhvk]

I think this will go wrong, since you now have a unit and then in the next clause multiply with u.deg, which keeps it a unit, and I think ra.to(u.deg).value will go wrong too (though I'm not sure how this ever could have worked??

I think the whole routine could just be

def _validate_ra(self, ra):
    ra = Quantity(ra, u.deg)
    if not (0 <= ra.value <= 360):
        raise ...

This will even support strings like "1.0 arcmin" (but not a simply unit string like "arcmin", which I think is correct).

[mhvk]

Here, again, converting to a unit seems wrong. One can just do low = Quantity(low), assuming that the string contains a unit too (otherwise, it is going to be dimensionless).

[bsipocz]

Thanks @mhvk for the review!

I would still like to have one of the other pyvo maintainers to have a look in case I missed a SODA case, something we should be able to handle but don't in fact support.