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

Improve generating Gaia ADQL queries #2831

Closed
wants to merge 2 commits into from

Conversation

eerovaher
Copy link
Member

@eerovaher eerovaher commented Sep 14, 2023

The first commit refactors how the Gaia ADQL queries are generated but does not change the generated queries in any way. The second commit removes useless whitespace which does change the query string but does not make any functional changes.

With 3b2110f (current main):

>>> from astroquery.gaia import Gaia
>>> print(Gaia.cone_search("Vega", radius="0.001deg", columns=["ra", "dec"]).parameters["query"])

                SELECT
                  TOP 50
                  ra,dec,
                  DISTANCE(
                    POINT('ICRS', ra, dec),
                    POINT('ICRS', 279.23473985606415, 38.78369481779005)
                  ) AS dist
                FROM
                  gaiadr3.gaia_source
                WHERE
                  1 = CONTAINS(
                    POINT('ICRS', ra, dec),
                    CIRCLE('ICRS', 279.23473985606415, 38.78369481779005, 0.001)
                  )
                ORDER BY
                  dist ASC
     

With b5bb8f0 (this pull request):

>>> from astroquery.gaia import Gaia
>>> print(Gaia.cone_search("Vega", radius="0.001deg", columns=["ra", "dec"]).parameters["query"])
SELECT
  TOP 50
  ra,dec,
  DISTANCE(
    POINT('ICRS', ra, dec),
    POINT('ICRS', 279.23473985606415, 38.78369481779005)
  ) AS dist
FROM
  gaiadr3.gaia_source
WHERE
  1 = CONTAINS(
    POINT('ICRS', ra, dec),
    CIRCLE('ICRS', 279.23473985606415, 38.78369481779005, 0.001)
  )
ORDER BY
  dist ASC

For reviewing the code changes it might be helpful to see a version of the diff that does not highlight lines where the only change was in indentation: https://github.com/astropy/astroquery/pull/2831/files?w=1

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #2831 (b5bb8f0) into main (3b2110f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2831      +/-   ##
==========================================
- Coverage   66.34%   66.34%   -0.01%     
==========================================
  Files         235      235              
  Lines       18077    18068       -9     
==========================================
- Hits        11994    11987       -7     
+ Misses       6083     6081       -2     
Files Changed Coverage Δ
astroquery/gaia/core.py 70.54% <100.00%> (-0.23%) ⬇️

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

@eerovaher
Copy link
Member Author

eerovaher commented Sep 14, 2023

@jespinosaar has requested that @esdc-esac-esa-int should be notified when any ESA code is changed.

@bsipocz
Copy link
Member

bsipocz commented Sep 14, 2023

be registered as code owners so that GitHub would notify them automatically.

@eerovaher - we do not do code owners here, so please refrain from making such suggestions.

@eerovaher
Copy link
Member Author

eerovaher commented Sep 14, 2023

If you don't set up code owners to notify relevant people automatically then a maintainer needs to do it manually. So why wouldn't you want to set up code owners?

@keflavich
Copy link
Contributor

@eerovaher Thanks for the code style change suggestions here. Because @esdc-esac-esa-int is maintaining this module, though, we prefer to let them decide on relatively minor code style choices like those made here. @esdc-esac-esa-int and @jespinosaar, please do let us know if and where this sort of style recommendation is best made.

Regarding code owners, and other github features, we are not generally adopting all of the latest features, and this one is not appropriate for astroquery anyway - permissions within astropy are handled at the organization level, and we're only adding write permissions when necessary.

For now, I'm going to close this PR, but we can reopen if @esdc-esac-esa-int request it.

@keflavich keflavich closed this Sep 14, 2023
@eerovaher
Copy link
Member Author

It didn't occur to me that code owners must have write access until @keflavich mentioned it. Now that I've learned this fact it is indeed simple to understand why astroquery doesn't want to use that feature.

@jespinosaar
Copy link
Contributor

Hi all,

Please let me contact Gaia team to check this. Thanks for putting esdc account in copy!

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

Successfully merging this pull request may close these issues.

4 participants