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

feat: add intersect modes for registry.Spatial constraint #495

Conversation

ManonMarchand
Copy link
Member

@ManonMarchand ManonMarchand commented Oct 19, 2023

Hello pyvo,

What the PR does

It adds intersect modes for the constraint registry.Spatial.

  • 'covers' that keeps the previous behavior,
  • 'enclosed' is for services covering the given region,
  • 'overlaps' returns services intersecting with the region

In practice, if I give the allsky MOC, it looks like this:

(former behavior, now default. Returns any service that contains allsky)

>>> from astroquery.pyvo import registry
>>> result = registry.search(registry.Spatial("0/0-11"))
>>> len(result)
1147

(new behavior, returns any service overlapping with allsky)

>>> from astroquery.pyvo import registry
>>> result = registry.search(registry.Spatial("0/0-11", intersect="overlaps"))
>>> len(result)
18938

Why this API

I copied the SIA one

intersect : str
but there might be better options ?

@ManonMarchand ManonMarchand marked this pull request as draft October 19, 2023 17:02
@ManonMarchand ManonMarchand force-pushed the feat-alternative-contains-on-spatial-constraints branch 2 times, most recently from 0c2874f to a324432 Compare October 20, 2023 07:15
@msdemlei
Copy link
Contributor

For the record, I like this as-is (added value: it's a nice demo of why we want constraint classes rather than simple keywords). Thanks!

Let me know when you'd like a review.

@ManonMarchand ManonMarchand force-pushed the feat-alternative-contains-on-spatial-constraints branch from a324432 to 323f13f Compare October 20, 2023 07:42
@ManonMarchand ManonMarchand marked this pull request as ready for review October 20, 2023 07:49
@ManonMarchand
Copy link
Member Author

ManonMarchand commented Oct 20, 2023

Thanks, I'm ready for the review, but the failing test is not something I changed. Maybe linked to this PR in astropy?

If I can give feedback on the constraints, I like that they are classes a lot, however their names are a bit confusing (registry.Spatial does not scream constraint to me and I'd have preferred something along registry.SpatialConstraint) but once I found them, they are nice to use.

Copy link
Contributor

@msdemlei msdemlei left a comment

Choose a reason for hiding this comment

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

Looks good to me (and the changelog failure indeed seems unrelated).

Thanks!

@bsipocz
Copy link
Member

bsipocz commented Oct 20, 2023

Indeed, I haven't yet follow-ed up those astropy changes in pyvo. Opening a separate PR for it.

CHANGES.rst Outdated Show resolved Hide resolved
@bsipocz bsipocz added this to the v1.5 milestone Oct 20, 2023
@bsipocz
Copy link
Member

bsipocz commented Oct 20, 2023

fyi: the changelog check was failed as the milestone wasn't yet set. I'll think about a solution of how to make that check more flexible.

@ManonMarchand ManonMarchand force-pushed the feat-alternative-contains-on-spatial-constraints branch 3 times, most recently from 20adc21 to 87f1ff3 Compare October 24, 2023 14:17
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #495 (87f1ff3) into main (ad84a0e) will increase coverage by 0.00%.
Report is 5 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 87f1ff3 differs from pull request most recent head 6636d13. Consider uploading reports for the commit 6636d13 to get more accurate results

@@           Coverage Diff           @@
##             main     #495   +/-   ##
=======================================
  Coverage   80.07%   80.08%           
=======================================
  Files          52       52           
  Lines        6059     6071   +12     
=======================================
+ Hits         4852     4862   +10     
- Misses       1207     1209    +2     
Files Coverage Δ
pyvo/registry/rtcons.py 97.45% <100.00%> (-0.38%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

the three modes are 'covers' that keeps the previous behavior, 'enclosed' for services covering the given region and 'overlaps' for services intersecting with the region
@ManonMarchand ManonMarchand force-pushed the feat-alternative-contains-on-spatial-constraints branch from 87f1ff3 to 6636d13 Compare October 24, 2023 14:29
@bsipocz bsipocz merged commit 7682a18 into astropy:main Oct 24, 2023
10 checks passed
@bsipocz
Copy link
Member

bsipocz commented Oct 24, 2023

Thank you @ManonMarchand!

@ManonMarchand ManonMarchand deleted the feat-alternative-contains-on-spatial-constraints branch October 26, 2023 14:36
_extra_tables = ["rr.stc_spatial"]

takes_sequence = True

def __init__(self, geom_spec, order=6):
def __init__(self, geom_spec, intersect="covers", order=6):
Copy link
Member

Choose a reason for hiding this comment

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

This added an unnoticed backward incompatible API change. I'll open a PR to remedy this before rolling out v1.5, and ultimately #507 will address any similar future issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops sorry

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, this should be something to spot during the review. But again, a prime example of why I'm pushing for #507 as we shouldn't really be catering for the sole usage of positional arguments for everything.

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

Successfully merging this pull request may close these issues.

None yet

3 participants