Support Astropy Quantity as radius arg for Registry SkyCoord Spatial constraint

[duytnguyendtn]

Hi PyVO maintainers!

This PR adds a small quality of life improvement to the Spatial registry constraint, to allow an astropy quantity to be provided for the radius argument. Was implementing this downstream in Jdaviz, but figured it would be broadly useful here as well?

I've added a simple test to verify the unit conversion back to degrees, as well as testing for invalid units.

I'll add a changelog entry once a PR number is assigned.

Thanks!
Duy

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.47%. Comparing base (1c8741c) to head (13e4abe).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #594      +/-   ##
==========================================
+ Coverage   81.46%   81.47%   +0.01%     
==========================================
  Files          68       68              
  Lines        7034     7039       +5     
==========================================
+ Hits         5730     5735       +5     
  Misses       1304     1304              

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

[bsipocz]

Please update the error message as e.g. degree works there just as well as radians.

[bsipocz]

Please add this case to the docstring, and possibly an example to the narrative docs

[duytnguyendtn]

Tried my best to write useful examples!

Docstring example implemented in: be70c43

Narrative example implemented in: d91c80a

How do those look?

[bsipocz]

Re narrative docs: There is value to list the expected outputs, so I'll add those to your example, as well as will remove the IGNORE_OUTPUT, as we use that only for cases where results are expected to chang often or are unpredictable.

[bsipocz]

(ahh, never mind, I see that we already have that in the previous example)

[bsipocz]

I'll look into the (unrelated) CI failure separately. In the meantime, would you mind squashing the commits a bit to remove the incremental convergence done in the last ~4?

(you can also run test locally either using the same tox commands the CI does, or by running pytest pyvo -R and flake8 pyvo, those would smoke out most of these.

[duytnguyendtn]

Ahh does PyVO not usually squash PR commits at merge time? I'm admittedly reckless when it comes to my commits due to the assumption they'll get squashed anyways when it gets accepted. I'll clean up the commit history now!

[bsipocz]

Ahh does PyVO not usually squash PR commits at merge time

No, we assume nice logical chunks for commits 😅. It takes some time to get used to, but comes second nature soon enough. (and given that we have large PRs, I find it useful to have them merged in in a manageable sequence into main, it will make it easier to dig back history/bugs as opposed to have one gigantic commit on merge and then a likely messy commit history separately in a PR).

So TL;DR: no need to squash down to 1 commit, but limit the one-liner CI wrangler ones once a PR is ready for review/merge.

[bsipocz]

Thank you @duytnguyendtn!

[bsipocz]

Having a look at this, we can interpret this PR as a bugfix, as quantities should have been supported already. So I'll remilestone, which will also help the jdaviz folks.